From 54c514f24ee00d885ec633a137a78a4cc71c6781 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Wed, 29 Jun 2016 11:55:23 -0500 Subject: Add 2FA check to the OAuth authentication mechanism --- app/controllers/omniauth_callbacks_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index f35d631df0c..619a76ebfd9 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -107,6 +107,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController # Only allow properly saved users to login. if @user.persisted? && @user.valid? log_audit_event(@user, with: oauth['provider']) + prompt_for_two_factor(@user) and return if @user.two_factor_enabled? sign_in_and_redirect(@user) else error_message = @user.errors.full_messages.to_sentence -- cgit v1.2.1 From de1578185ecc0a439d877dbe1bc39a15e653c04b Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 1 Jul 2016 18:09:17 +0200 Subject: Remove icons from some buttons which already has text Signed-off-by: Dmitriy Zaporozhets --- app/views/groups/milestones/index.html.haml | 1 - app/views/groups/projects.html.haml | 1 - app/views/shared/milestones/_summary.html.haml | 1 - 3 files changed, 3 deletions(-) diff --git a/app/views/groups/milestones/index.html.haml b/app/views/groups/milestones/index.html.haml index 121a7de3ad7..a8fdbd8c426 100644 --- a/app/views/groups/milestones/index.html.haml +++ b/app/views/groups/milestones/index.html.haml @@ -6,7 +6,6 @@ .nav-controls - if can?(current_user, :admin_milestones, @group) = link_to new_group_milestone_path(@group), class: "btn btn-new" do - = icon('plus') New Milestone .row-content-block diff --git a/app/views/groups/projects.html.haml b/app/views/groups/projects.html.haml index c2f2d9912f7..33fee334d93 100644 --- a/app/views/groups/projects.html.haml +++ b/app/views/groups/projects.html.haml @@ -7,7 +7,6 @@ - if can? current_user, :admin_group, @group .controls = link_to new_project_path(namespace_id: @group.id), class: "btn btn-sm btn-success" do - = icon('plus') New Project %ul.well-list - @projects.each do |project| diff --git a/app/views/shared/milestones/_summary.html.haml b/app/views/shared/milestones/_summary.html.haml index 975c74f4ea6..dee2472fa79 100644 --- a/app/views/shared/milestones/_summary.html.haml +++ b/app/views/shared/milestones/_summary.html.haml @@ -26,7 +26,6 @@ %span.pull-right.tab-issues-buttons - if project && can?(current_user, :create_issue, project) = link_to new_namespace_project_issue_path(project.namespace, project, issue: { milestone_id: milestone.id }), class: "btn btn-grouped", title: "New Issue" do - %i.fa.fa-plus New Issue = link_to 'Browse Issues', milestones_browse_issuables_path(milestone, type: :issues), class: "btn btn-grouped" %span.pull-right.tab-merge-requests-buttons.hidden -- cgit v1.2.1 From 5467260528018774c8baec65f3cbf692bb3d93b7 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Thu, 30 Jun 2016 14:54:07 -0500 Subject: Added tests for 2FA check on OAuth request --- app/controllers/omniauth_callbacks_controller.rb | 7 +++++-- spec/features/login_spec.rb | 26 +++++++++++++++++++----- spec/spec_helper.rb | 2 ++ spec/support/login_helpers.rb | 25 +++++++++++++++++++++++ 4 files changed, 53 insertions(+), 7 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 619a76ebfd9..f54c79c2e37 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -107,8 +107,11 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController # Only allow properly saved users to login. if @user.persisted? && @user.valid? log_audit_event(@user, with: oauth['provider']) - prompt_for_two_factor(@user) and return if @user.two_factor_enabled? - sign_in_and_redirect(@user) + if @user.two_factor_enabled? + prompt_for_two_factor(@user) + else + sign_in_and_redirect(@user) + end else error_message = @user.errors.full_messages.to_sentence diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index 72b5ff231f7..c3dfe343052 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -28,6 +28,11 @@ feature 'Login', feature: true do end describe 'with two-factor authentication' do + def enter_code(code) + fill_in 'Two-Factor Authentication code', with: code + click_button 'Verify code' + end + context 'with valid username/password' do let(:user) { create(:user, :two_factor) } @@ -36,11 +41,6 @@ feature 'Login', feature: true do expect(page).to have_content('Two-Factor Authentication') end - def enter_code(code) - fill_in 'Two-Factor Authentication code', with: code - click_button 'Verify code' - end - it 'does not show a "You are already signed in." error message' do enter_code(user.current_otp) expect(page).not_to have_content('You are already signed in.') @@ -108,6 +108,22 @@ feature 'Login', feature: true do end end end + + context 'logging in via OAuth' do + def stub_omniauth_config(messages) + allow(Gitlab.config.omniauth).to receive_messages(messages) + end + + it 'should show 2FA prompt after OAuth login' do + user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') + stub_omniauth_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [OpenStruct.new(name: 'saml', label: 'saml', args: {})]) + login_via('saml', user, 'my-uid') + + expect(page).to have_content('Two-Factor Authentication') + enter_code(user.current_otp) + expect(current_path).to eq root_path + end + end end describe 'without two-factor authentication' do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b43f38ef202..537aa46a2fd 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -57,3 +57,5 @@ FactoryGirl::SyntaxRunner.class_eval do end ActiveRecord::Migration.maintain_test_schema! + +OmniAuth.config.test_mode = true diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 7a0f078c72b..75a8846c0da 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -37,6 +37,31 @@ module LoginHelpers Thread.current[:current_user] = user end + def login_via(provider, user, uid) + mock_auth_hash(provider, uid, user.email) + visit new_user_session_path + #page.find('.oauth-image-link').click + click_link provider + end + + def mock_auth_hash(provider, uid, email) + # The mock_auth configuration allows you to set per-provider (or default) + # authentication hashes to return during integration testing. + OmniAuth.config.mock_auth[provider.to_sym] = OmniAuth::AuthHash.new({ + provider: provider, + uid: uid, + info: { + name: 'mockuser', + email: email, + image: 'mock_user_thumbnail_url' + }, + credentials: { + token: 'mock_token', + secret: 'mock_secret' + } + }) + end + # Requires Javascript driver. def logout find(:css, ".fa.fa-sign-out").click -- cgit v1.2.1 From 40e16b22f9a4ecb7dd7a25c4a0355809bed70ebe Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Mon, 4 Jul 2016 11:50:11 -0500 Subject: Change order of stubbing to fix tests --- spec/features/login_spec.rb | 2 +- spec/support/login_helpers.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index c3dfe343052..39968db5b58 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -115,8 +115,8 @@ feature 'Login', feature: true do end it 'should show 2FA prompt after OAuth login' do - user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') stub_omniauth_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [OpenStruct.new(name: 'saml', label: 'saml', args: {})]) + user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') login_via('saml', user, 'my-uid') expect(page).to have_content('Two-Factor Authentication') diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 75a8846c0da..f9ce929000c 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -40,7 +40,6 @@ module LoginHelpers def login_via(provider, user, uid) mock_auth_hash(provider, uid, user.email) visit new_user_session_path - #page.find('.oauth-image-link').click click_link provider end -- cgit v1.2.1 From 511c79f094a1c2f366fb43d7d05f746b8adccd62 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Tue, 5 Jul 2016 14:21:14 +0200 Subject: Remove hound config --- .hound.yml | 4 ---- 1 file changed, 4 deletions(-) delete mode 100644 .hound.yml diff --git a/.hound.yml b/.hound.yml deleted file mode 100644 index 3bde29fb2bf..00000000000 --- a/.hound.yml +++ /dev/null @@ -1,4 +0,0 @@ -# Prefer single quotes -StringLiterals: - EnforcedStyle: single_quotes - Enabled: true -- cgit v1.2.1 From a0c8bd33a924cc38f435c733a9306f39f9c8cce7 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Tue, 5 Jul 2016 19:35:34 +0200 Subject: Remove teatro config --- .teatro.yml | 8 ----- config/gitlab.teatro.yml | 87 ------------------------------------------------ 2 files changed, 95 deletions(-) delete mode 100644 .teatro.yml delete mode 100644 config/gitlab.teatro.yml diff --git a/.teatro.yml b/.teatro.yml deleted file mode 100644 index 30054361981..00000000000 --- a/.teatro.yml +++ /dev/null @@ -1,8 +0,0 @@ -stage: - before: - - cp config/gitlab.teatro.yml config/gitlab.yml - - mkdir /apps/gitlab-satellites - - mkdir /apps/repositories - - database: - - RAILS_ENV=development force=yes bundle exec rake db:create gitlab:setup \ No newline at end of file diff --git a/config/gitlab.teatro.yml b/config/gitlab.teatro.yml deleted file mode 100644 index 75b79b837e0..00000000000 --- a/config/gitlab.teatro.yml +++ /dev/null @@ -1,87 +0,0 @@ - -production: &base - gitlab: - host: localhost - port: 80 - https: false - - user: root - - email_from: example@example.com - - support_email: support@example.com - - default_projects_features: - issues: true - merge_requests: true - wiki: true - snippets: false - visibility_level: "private" # can be "private" | "internal" | "public" - - issues_tracker: - - gravatar: - enabled: true # Use user avatar image from Gravatar.com (default: true) - - ldap: - enabled: false - host: '_your_ldap_server' - port: 636 - uid: 'sAMAccountName' - method: 'ssl' # "tls" or "ssl" or "plain" - bind_dn: '_the_full_dn_of_the_user_you_will_bind_with' - password: '_the_password_of_the_bind_user' - allow_username_or_email_login: true - - base: '' - - user_filter: '' - - omniauth: - enabled: false - - satellites: - # Relative paths are relative to Rails.root (default: tmp/repo_satellites/) - path: /apps/gitlab-satellites/ - - backup: - path: "tmp/backups" # Relative paths are relative to Rails.root (default: tmp/backups/) - - repositories: - storages: # REPO PATHS MUST NOT BE A SYMLINK!!! - default: /apps/repositories/ - - gitlab_shell: - path: /apps/gitlab-shell/ - - hooks_path: /apps/gitlab-shell/hooks/ - - upload_pack: true - receive_pack: true - - git: - bin_path: /usr/bin/git - max_size: 5242880 # 5.megabytes - timeout: 10 - - extra: - -development: - <<: *base - -test: - <<: *base - gravatar: - enabled: true - gitlab: - host: localhost - port: 80 - issues_tracker: - redmine: - title: "Redmine" - project_url: "http://redmine/projects/:issues_tracker_id" - issues_url: "http://redmine/:project_id/:issues_tracker_id/:id" - new_issue_url: "http://redmine/projects/:issues_tracker_id/issues/new" - -staging: - <<: *base -- cgit v1.2.1 From c8407dd9cfa970de698ac50a0af636b1fa8df0b0 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Wed, 6 Jul 2016 12:15:28 -0500 Subject: Change new pipeline to run pipeline --- app/views/projects/pipelines/index.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/pipelines/index.html.haml b/app/views/projects/pipelines/index.html.haml index 28b475d5c2f..a1b8454ecda 100644 --- a/app/views/projects/pipelines/index.html.haml +++ b/app/views/projects/pipelines/index.html.haml @@ -28,7 +28,7 @@ .nav-controls - if can? current_user, :create_pipeline, @project = link_to new_namespace_project_pipeline_path(@project.namespace, @project), class: 'btn btn-create' do - New pipeline + Run pipeline - unless @repository.gitlab_ci_yml = link_to 'Get started with Pipelines', help_page_path('ci/quick_start', 'README'), class: 'btn btn-info' -- cgit v1.2.1 From eda205342140e1bc71b5f0b6935b5ffc502a85cd Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Mon, 4 Jul 2016 18:29:10 -0500 Subject: Move Omniauth specific setup to its own file --- spec/spec_helper.rb | 4 +--- spec/support/omni_auth.rb | 1 + 2 files changed, 2 insertions(+), 3 deletions(-) create mode 100644 spec/support/omni_auth.rb diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 537aa46a2fd..09ff5a18a35 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -56,6 +56,4 @@ FactoryGirl::SyntaxRunner.class_eval do include RSpec::Mocks::ExampleMethods end -ActiveRecord::Migration.maintain_test_schema! - -OmniAuth.config.test_mode = true +ActiveRecord::Migration.maintain_test_schema! \ No newline at end of file diff --git a/spec/support/omni_auth.rb b/spec/support/omni_auth.rb new file mode 100644 index 00000000000..3d262ff9ca0 --- /dev/null +++ b/spec/support/omni_auth.rb @@ -0,0 +1 @@ +OmniAuth.config.test_mode = true \ No newline at end of file -- cgit v1.2.1 From 2a0be666e3078e28a02de298b386ec4c09232978 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Tue, 5 Jul 2016 16:34:34 -0500 Subject: Added a lot of stubbing to make sure OAUth requests are handled properly with 2FA --- spec/features/login_spec.rb | 19 ++++++++++++++++++- spec/spec_helper.rb | 2 +- spec/support/login_helpers.rb | 10 ++++++++++ spec/support/omni_auth.rb | 2 +- 4 files changed, 30 insertions(+), 3 deletions(-) diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index 39968db5b58..58753ff21f6 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -110,12 +110,29 @@ feature 'Login', feature: true do end context 'logging in via OAuth' do + def saml_config + OpenStruct.new(name: 'saml', label: 'saml', args: { + assertion_consumer_service_url: 'https://localhost:3443/users/auth/saml/callback', + idp_cert_fingerprint: '26:43:2C:47:AF:F0:6B:D0:07:9C:AD:A3:74:FE:5D:94:5F:4E:9E:52', + idp_sso_target_url: 'https://idp.example.com/sso/saml', + issuer: 'https://localhost:3443/', + name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' + }) + end + def stub_omniauth_config(messages) + Rails.application.env_config['devise.mapping'] = Devise.mappings[:user] + Rails.application.routes.disable_clear_and_finalize = true + Rails.application.routes.draw do + post '/users/auth/saml' => 'omniauth_callbacks#saml' + end + allow(Gitlab::OAuth::Provider).to receive_messages(providers: [:saml], config_for: saml_config) allow(Gitlab.config.omniauth).to receive_messages(messages) + allow_any_instance_of(Object).to receive(:user_omniauth_authorize_path).with('saml').and_return('/users/auth/saml') end it 'should show 2FA prompt after OAuth login' do - stub_omniauth_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [OpenStruct.new(name: 'saml', label: 'saml', args: {})]) + stub_omniauth_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [saml_config]) user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') login_via('saml', user, 'my-uid') diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 09ff5a18a35..b43f38ef202 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -56,4 +56,4 @@ FactoryGirl::SyntaxRunner.class_eval do include RSpec::Mocks::ExampleMethods end -ActiveRecord::Migration.maintain_test_schema! \ No newline at end of file +ActiveRecord::Migration.maintain_test_schema! diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index f9ce929000c..2e400dd825a 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -39,6 +39,7 @@ module LoginHelpers def login_via(provider, user, uid) mock_auth_hash(provider, uid, user.email) + Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[:saml] visit new_user_session_path click_link provider end @@ -57,6 +58,15 @@ module LoginHelpers credentials: { token: 'mock_token', secret: 'mock_secret' + }, + extra: { + raw_info: { + info: { + name: 'mockuser', + email: email, + image: 'mock_user_thumbnail_url' + } + } } }) end diff --git a/spec/support/omni_auth.rb b/spec/support/omni_auth.rb index 3d262ff9ca0..0b1af4052ff 100644 --- a/spec/support/omni_auth.rb +++ b/spec/support/omni_auth.rb @@ -1 +1 @@ -OmniAuth.config.test_mode = true \ No newline at end of file +OmniAuth.config.test_mode = true -- cgit v1.2.1 From d2f003a344e6f29a0ad5115c7d8dec5727b87895 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Mon, 11 Jul 2016 09:06:36 -0700 Subject: Update the health_check gem to the latest release This allows us to drop our disable email config override --- CHANGELOG | 1 + Gemfile | 2 +- Gemfile.lock | 6 +++--- config/initializers/health_check.rb | 13 ------------- 4 files changed, 5 insertions(+), 17 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index a977fc3fdbf..8bd7b388daa 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -26,6 +26,7 @@ v 8.10.0 (unreleased) - Exclude email check from the standard health check - Updated layout for Projects, Groups, Users on Admin area !4424 - Fix changing issue state columns in milestone view + - Update health_check gem to version 2.1.0 - Add notification settings dropdown for groups - Wildcards for protected branches. !4665 - Allow importing from Github using Personal Access Tokens. (Eric K Idema) diff --git a/Gemfile b/Gemfile index f1fef4caf76..5c43015e52c 100644 --- a/Gemfile +++ b/Gemfile @@ -344,7 +344,7 @@ gem 'oauth2', '~> 1.2.0' gem 'paranoia', '~> 2.0' # Health check -gem 'health_check', '~> 1.5.1' +gem 'health_check', '~> 2.1.0' # System information gem 'vmstat', '~> 2.1.0' diff --git a/Gemfile.lock b/Gemfile.lock index 721ab9ddc5d..f8018e58a5e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -322,8 +322,8 @@ GEM thor tilt hashie (3.4.3) - health_check (1.5.1) - rails (>= 2.3.0) + health_check (2.1.0) + rails (>= 4.0) hipchat (1.5.2) httparty mimemagic @@ -870,7 +870,7 @@ DEPENDENCIES grape (~> 0.13.0) grape-entity (~> 0.4.2) hamlit (~> 2.5) - health_check (~> 1.5.1) + health_check (~> 2.1.0) hipchat (~> 1.5.0) html-pipeline (~> 1.11.0) httparty (~> 0.13.3) diff --git a/config/initializers/health_check.rb b/config/initializers/health_check.rb index 6796407d4e6..4c91a61fb4a 100644 --- a/config/initializers/health_check.rb +++ b/config/initializers/health_check.rb @@ -1,16 +1,3 @@ -# Email forcibly included in the standard checks, but the email health check -# doesn't support the full range of SMTP options, which can result in failures -# for valid SMTP configurations. -# Overwrite the HealthCheck's detection of whether email is configured -# in order to avoid the email check during standard checks -module HealthCheck - class Utils - def self.mailer_configured? - false - end - end -end - HealthCheck.setup do |config| config.standard_checks = ['database', 'migrations', 'cache'] config.full_checks = ['database', 'migrations', 'cache'] -- cgit v1.2.1 From 7040c1ba3e79cd68d6084aba88d670067835b2f3 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 11 Jul 2016 19:37:28 +0300 Subject: Rename profile navigation tab 'Personal Access Tokes' to 'Access Tokens' Signed-off-by: Dmitriy Zaporozhets --- app/views/layouts/nav/_profile.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/layouts/nav/_profile.html.haml b/app/views/layouts/nav/_profile.html.haml index 96fe62c39c3..6e3c9ab9778 100644 --- a/app/views/layouts/nav/_profile.html.haml +++ b/app/views/layouts/nav/_profile.html.haml @@ -20,7 +20,7 @@ = nav_link(controller: :personal_access_tokens) do = link_to profile_personal_access_tokens_path, title: 'Personal Access Tokens' do %span - Personal Access Tokens + Access Tokens = nav_link(controller: :emails) do = link_to profile_emails_path, title: 'Emails' do %span -- cgit v1.2.1 From 4765a3414e90d1bdfbbd1f9d42decedef9d137cd Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 11 Jul 2016 19:51:20 +0300 Subject: Add rule about adding new header tab to the ui guide Signed-off-by: Dmitriy Zaporozhets --- doc/development/ui_guide.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/development/ui_guide.md b/doc/development/ui_guide.md index ce0aaa2fd25..b018a8c66d3 100644 --- a/doc/development/ui_guide.md +++ b/doc/development/ui_guide.md @@ -27,6 +27,8 @@ We try to keep the amount of tabs in the header navigation between 5 and 10 so t tab should represent separate functionality. Everything related to the issue tracker should be under the 'Issues' tab while everything related to the wiki should be under 'Wiki' tab and so on and so forth. +When add new tab to the header don't use more than 2 words for text in the link. +We want to keep links short and easy to remmeber and fit all of them in the small screen. ## Mobile screen size -- cgit v1.2.1 From c4fb58ef35183fa8eeb4eee2d065f2d66ea75454 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 11 Jul 2016 16:54:24 +0000 Subject: Fix typo in UI guide --- doc/development/ui_guide.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/development/ui_guide.md b/doc/development/ui_guide.md index b018a8c66d3..3c44affdd76 100644 --- a/doc/development/ui_guide.md +++ b/doc/development/ui_guide.md @@ -28,7 +28,7 @@ tab should represent separate functionality. Everything related to the issue tracker should be under the 'Issues' tab while everything related to the wiki should be under 'Wiki' tab and so on and so forth. When add new tab to the header don't use more than 2 words for text in the link. -We want to keep links short and easy to remmeber and fit all of them in the small screen. +We want to keep links short and easy to remember and fit all of them in the small screen. ## Mobile screen size -- cgit v1.2.1 From 24cf6b9f62a312c010c9479fd6155f7c72099979 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Mon, 11 Jul 2016 12:41:02 -0500 Subject: Refactor `mock_auth_hash` --- spec/support/login_helpers.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 2e400dd825a..ba08733e48d 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -39,7 +39,6 @@ module LoginHelpers def login_via(provider, user, uid) mock_auth_hash(provider, uid, user.email) - Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[:saml] visit new_user_session_path click_link provider end @@ -69,6 +68,7 @@ module LoginHelpers } } }) + Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[:saml] end # Requires Javascript driver. -- cgit v1.2.1 From eac19b289384a6281c75781e79bad21408063c1c Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Mon, 11 Jul 2016 11:25:01 -0400 Subject: api: expose {should,force}_remove_source_branch Workflows which use a bot to merge should remove branches if requested. Expose the flag so that bots can request know this. --- CHANGELOG | 1 + doc/api/merge_requests.md | 30 +++++++++++++++++++++++------- lib/api/entities.rb | 2 ++ spec/requests/api/merge_requests_spec.rb | 4 ++++ 4 files changed, 30 insertions(+), 7 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index a977fc3fdbf..41c6884a2a0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.10.0 (unreleased) + - Expose {should,force}_remove_source_branch (Ben Boeckel) - Fix commit builds API, return all builds for all pipelines for given commit. !4849 - Replace Haml with Hamlit to make view rendering faster. !3666 - Refactor repository paths handling to allow multiple git mount points diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 816f09e1007..a8c3b068d22 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -68,7 +68,9 @@ Parameters: "merge_when_build_succeeds": true, "merge_status": "can_be_merged", "subscribed" : false, - "user_notes_count": 1 + "user_notes_count": 1, + "should_remove_source_branch": true, + "force_remove_source_branch": false } ] ``` @@ -132,7 +134,9 @@ Parameters: "merge_when_build_succeeds": true, "merge_status": "can_be_merged", "subscribed" : true, - "user_notes_count": 1 + "user_notes_count": 1, + "should_remove_source_branch": true, + "force_remove_source_branch": false } ``` @@ -233,6 +237,8 @@ Parameters: "merge_status": "can_be_merged", "subscribed" : true, "user_notes_count": 1, + "should_remove_source_branch": true, + "force_remove_source_branch": false, "changes": [ { "old_path": "VERSION", @@ -312,7 +318,9 @@ Parameters: "merge_when_build_succeeds": true, "merge_status": "can_be_merged", "subscribed" : true, - "user_notes_count": 0 + "user_notes_count": 0, + "should_remove_source_branch": true, + "force_remove_source_branch": false } ``` @@ -383,7 +391,9 @@ Parameters: "merge_when_build_succeeds": true, "merge_status": "can_be_merged", "subscribed" : true, - "user_notes_count": 1 + "user_notes_count": 1, + "should_remove_source_branch": true, + "force_remove_source_branch": false } ``` @@ -481,7 +491,9 @@ Parameters: "merge_when_build_succeeds": true, "merge_status": "can_be_merged", "subscribed" : true, - "user_notes_count": 1 + "user_notes_count": 1, + "should_remove_source_branch": true, + "force_remove_source_branch": false } ``` @@ -547,7 +559,9 @@ Parameters: "merge_when_build_succeeds": true, "merge_status": "can_be_merged", "subscribed" : true, - "user_notes_count": 1 + "user_notes_count": 1, + "should_remove_source_branch": true, + "force_remove_source_branch": false } ``` @@ -866,7 +880,9 @@ Example response: "merge_when_build_succeeds": false, "merge_status": "unchecked", "subscribed": true, - "user_notes_count": 7 + "user_notes_count": 7, + "should_remove_source_branch": true, + "force_remove_source_branch": false }, "target_url": "https://gitlab.example.com/gitlab-org/gitlab-ci/merge_requests/7", "body": "Et voluptas laudantium minus nihil recusandae ut accusamus earum aut non.", diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 9076a0c3831..d8e987ee8d4 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -208,6 +208,8 @@ module API merge_request.subscribed?(options[:current_user]) end expose :user_notes_count + expose :should_remove_source_branch?, as: :should_remove_source_branch + expose :force_remove_source_branch?, as: :force_remove_source_branch end class MergeRequestChanges < MergeRequest diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 4a1b5600bdf..651b91e9f68 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -138,6 +138,8 @@ describe API::API, api: true do expect(json_response['work_in_progress']).to be_falsy expect(json_response['merge_when_build_succeeds']).to be_falsy expect(json_response['merge_status']).to eq('can_be_merged') + expect(json_response['should_close_merge_request']).to be_falsy + expect(json_response['force_close_merge_request']).to be_falsy end it "should return merge_request" do @@ -147,6 +149,8 @@ describe API::API, api: true do expect(json_response['iid']).to eq(merge_request.iid) expect(json_response['work_in_progress']).to eq(false) expect(json_response['merge_status']).to eq('can_be_merged') + expect(json_response['should_close_merge_request']).to be_falsy + expect(json_response['force_close_merge_request']).to be_falsy end it 'should return merge_request by iid' do -- cgit v1.2.1 From ad7cca067cc3c6ace8da2433bee9d8cecd2b6f42 Mon Sep 17 00:00:00 2001 From: Rasim Demirbay Date: Mon, 11 Jul 2016 17:32:01 +0300 Subject: Style of import project buttons were fixed in the new project page. --- app/assets/stylesheets/pages/projects.scss | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index bce4aac3334..5be911dc562 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -340,23 +340,30 @@ a.deploy-project-label { .project-import { .form-group { - margin-bottom: 0; + margin-bottom: 5px; } + .import-buttons { padding-left: 0; display: -webkit-flex; display: flex; -webkit-flex-wrap: wrap; flex-wrap: wrap; + .btn { - margin-right: 10px; - padding: 8px 12px; + margin: 0 10px 10px 0; + padding: 8px; } - &> div { - margin-bottom: 14px; + + > div { padding-left: 0; + &:last-child { margin-bottom: 0; + + .btn { + margin-right: 0; + } } } } -- cgit v1.2.1 From 3d16a50ee3d67c49ecfd4b997829c029c958105d Mon Sep 17 00:00:00 2001 From: Rasim Demirbay Date: Mon, 11 Jul 2016 18:12:25 +0300 Subject: CHANGELOG was updated. --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 0000e288b19..d1bf5d0ef9e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -66,6 +66,7 @@ v 8.10.0 (unreleased) - Add min value for project limit field on user's form !3622 (jastkand) - Add reminder to not paste private SSH keys !4399 (Ingo Blechschmidt) - Remove duplicate `description` field in `MergeRequest` entities (Ben Boeckel) + - Style of import project buttons were fixed in the new project page. !5183 (rdemirbay) v 8.9.5 - Add more debug info to import/export and memory killer. !5108 -- cgit v1.2.1 From 05109ed8eaa4872a89be987d3a080489241a23c8 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Mon, 11 Jul 2016 16:16:57 -0500 Subject: Update header block class on snippets page --- app/views/projects/snippets/index.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/snippets/index.html.haml b/app/views/projects/snippets/index.html.haml index 6c994ae486b..1646bcf4b8a 100644 --- a/app/views/projects/snippets/index.html.haml +++ b/app/views/projects/snippets/index.html.haml @@ -1,6 +1,6 @@ - page_title "Snippets" -.row-content-block.top-block +.sub-header-block .pull-right - if can?(current_user, :create_project_snippet, @project) = link_to new_namespace_project_snippet_path(@project.namespace, @project), class: "btn btn-new", title: "New Snippet" do -- cgit v1.2.1 From ce5a5f75e428bd9de7de0b126d16b922a157646f Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Mon, 11 Jul 2016 16:42:25 -0500 Subject: Add margin to filter labels --- app/assets/stylesheets/pages/labels.scss | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index 47bfd144930..3b1e38fc07d 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/app/assets/stylesheets/pages/labels.scss @@ -162,9 +162,15 @@ } .filtered-labels { + font-size: 0; + padding: 12px 16px; + .label-row { + margin-top: 4px; + margin-bottom: 4px; + &:not(:last-child) { - margin-right: 5px; + margin-right: 8px; } } -- cgit v1.2.1 From f5cc3f63a8a6a44e755aa81ac6cba8e544b848e6 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 11 Jul 2016 00:08:01 -0500 Subject: Render inline diffs for multiple changed lines following eachother --- CHANGELOG | 1 + lib/gitlab/diff/inline_diff.rb | 52 +++++++++++++++++++++++--------- spec/lib/gitlab/diff/inline_diff_spec.rb | 28 +++++++++++------ 3 files changed, 57 insertions(+), 24 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 3e4a10bb5a3..02cbd9bb295 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -25,6 +25,7 @@ v 8.10.0 (unreleased) - Updated layout for Projects, Groups, Users on Admin area !4424 - Fix changing issue state columns in milestone view - Add notification settings dropdown for groups + - Render inline diffs for multiple changed lines following eachother - Wildcards for protected branches. !4665 - Allow importing from Github using Personal Access Tokens. (Eric K Idema) - API: Todos !3188 (Robert Schilling) diff --git a/lib/gitlab/diff/inline_diff.rb b/lib/gitlab/diff/inline_diff.rb index 789c14518b0..72d9abeefcc 100644 --- a/lib/gitlab/diff/inline_diff.rb +++ b/lib/gitlab/diff/inline_diff.rb @@ -1,16 +1,30 @@ module Gitlab module Diff class InlineDiff + # Regex to find a run of deleted lines followed by the same number of added lines + REGEX = %r{ + # Runs start at the beginning of the string (the first line) or after a space (for an unchanged line) + (?:\A| ) + + # This matches a number of `-`s followed by the same number of `+`s through recursion + (? + - + \g? + \+ + ) + + # Runs end at the end of the string (the last line) or before a space (for an unchanged line) + (?= |\z) + }x.freeze + attr_accessor :old_line, :new_line, :offset def self.for_lines(lines) - local_edit_indexes = self.find_local_edits(lines) + changed_line_pairs = self.find_changed_line_pairs(lines) inline_diffs = [] - local_edit_indexes.each do |index| - old_index = index - new_index = index + 1 + changed_line_pairs.each do |old_index, new_index| old_line = lines[old_index] new_line = lines[new_index] @@ -51,18 +65,28 @@ module Gitlab private - def self.find_local_edits(lines) - line_prefixes = lines.map { |line| line.match(/\A([+-])/) ? $1 : ' ' } - joined_line_prefixes = " #{line_prefixes.join} " - - offset = 0 - local_edit_indexes = [] - while index = joined_line_prefixes.index(" -+ ", offset) - local_edit_indexes << index - offset = index + 1 + # Finds pairs of old/new line pairs that represent the same line that changed + def self.find_changed_line_pairs(lines) + # Prefixes of all diff lines, indicating their types + # For example: `" - + -+ ---+++ --+ -++"` + line_prefixes = lines.each_with_object("") { |line, s| s << line[0] }.gsub(/[^ +-]/, ' ') + + changed_line_pairs = [] + line_prefixes.scan(REGEX) do + # For `"---+++"`, `begin_index == 0`, `end_index == 6` + begin_index, end_index = Regexp.last_match.offset(:del_ins) + + # For `"---+++"`, `changed_line_count == 3` + changed_line_count = (end_index - begin_index) / 2 + + halfway_index = begin_index + changed_line_count + (begin_index...halfway_index).each do |i| + # For `"---+++"`, index 1 maps to 1 + 3 = 4 + changed_line_pairs << [i, i + changed_line_count] + end end - local_edit_indexes + changed_line_pairs end def longest_common_prefix(a, b) diff --git a/spec/lib/gitlab/diff/inline_diff_spec.rb b/spec/lib/gitlab/diff/inline_diff_spec.rb index 95a993d26cf..8ca3f73509e 100644 --- a/spec/lib/gitlab/diff/inline_diff_spec.rb +++ b/spec/lib/gitlab/diff/inline_diff_spec.rb @@ -3,14 +3,19 @@ require 'spec_helper' describe Gitlab::Diff::InlineDiff, lib: true do describe '.for_lines' do let(:diff) do - < Date: Tue, 12 Jul 2016 13:24:51 +0800 Subject: Bump gitlab-workhorse version for !5094 --- GITLAB_WORKHORSE_VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index 879be8a98fc..e7c7d3cc3c8 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -0.7.7 +0.7.8 -- cgit v1.2.1 From cdc031b65b52f280defe46d278ce60432be895e1 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 12 Jul 2016 08:57:18 +0100 Subject: Fix expandable diffs --- app/assets/javascripts/diff.js.coffee | 1 + spec/features/expand_collapse_diffs_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/diff.js.coffee b/app/assets/javascripts/diff.js.coffee index 83516f97552..c132cc8c542 100644 --- a/app/assets/javascripts/diff.js.coffee +++ b/app/assets/javascripts/diff.js.coffee @@ -1,6 +1,7 @@ class @Diff UNFOLD_COUNT = 20 constructor: -> + $('.files .diff-file').singleFileDiff() @filesCommentButton = $('.files .diff-file').filesCommentButton() $(document).off('click', '.js-unfold') diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb index 7cff196c8d9..78bc888f2a6 100644 --- a/spec/features/expand_collapse_diffs_spec.rb +++ b/spec/features/expand_collapse_diffs_spec.rb @@ -106,7 +106,7 @@ feature 'Expand and collapse diffs', js: true, feature: true do let(:comment_text) { 'A comment' } before do - large_diff.find('.line_holder', match: :prefer_exact).hover + large_diff.find('.diff-line-num', match: :prefer_exact).hover large_diff.find('.add-diff-note').click large_diff.find('.note-textarea').send_keys comment_text large_diff.find_button('Comment').click @@ -161,7 +161,7 @@ feature 'Expand and collapse diffs', js: true, feature: true do end it 'does not make a new HTTP request' do - expect(evaluate_script('ajaxUris')).to be_empty + expect(evaluate_script('ajaxUris')).not_to include(a_string_matching('small_diff.md')) end end end @@ -199,7 +199,7 @@ feature 'Expand and collapse diffs', js: true, feature: true do end it 'does not make a new HTTP request' do - expect(evaluate_script('ajaxUris')).to be_empty + expect(evaluate_script('ajaxUris')).not_to include(a_string_matching('small_diff.md')) end end end -- cgit v1.2.1 From 7b43749e69fac0f50f5b17511c7caf1d819bb767 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 12 Jul 2016 17:53:50 +0800 Subject: Prefer string for describe as of: https://gitlab.com/gitlab-org/gitlab-ce/blob/b7ba5fa06bfb434c9227a2175f936fc31fd3444f/doc/development/gotchas.md#dont-describe-symbols --- spec/models/project_spec.rb | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 5a27ccbab0a..d2269854354 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -296,7 +296,7 @@ describe Project, models: true do end end - describe :update_merge_requests do + describe '#update_merge_requests' do let(:project) { create(:project) } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:key) { create(:key, user_id: project.owner.id) } @@ -345,7 +345,7 @@ describe Project, models: true do end end - describe :to_param do + describe '#to_param' do context 'with namespace' do before do @group = create :group, name: 'gitlab' @@ -356,7 +356,7 @@ describe Project, models: true do end end - describe :repository do + describe '#repository' do let(:project) { create(:project) } it 'should return valid repo' do @@ -364,7 +364,7 @@ describe Project, models: true do end end - describe :default_issues_tracker? do + describe '#default_issues_tracker?' do let(:project) { create(:project) } let(:ext_project) { create(:redmine_project) } @@ -377,7 +377,7 @@ describe Project, models: true do end end - describe :external_issue_tracker do + describe '#external_issue_tracker' do let(:project) { create(:project) } let(:ext_project) { create(:redmine_project) } @@ -418,7 +418,7 @@ describe Project, models: true do end end - describe :cache_has_external_issue_tracker do + describe '#cache_has_external_issue_tracker' do let(:project) { create(:project) } it 'stores true if there is any external_issue_tracker' do @@ -440,7 +440,7 @@ describe Project, models: true do end end - describe :open_branches do + describe '#open_branches' do let(:project) { create(:project) } before do @@ -517,7 +517,7 @@ describe Project, models: true do end end - describe :avatar_type do + describe '#avatar_type' do let(:project) { create(:project) } it 'should be true if avatar is image' do @@ -531,7 +531,7 @@ describe Project, models: true do end end - describe :avatar_url do + describe '#avatar_url' do subject { project.avatar_url } let(:project) { create(:project) } @@ -568,7 +568,7 @@ describe Project, models: true do end end - describe :pipeline do + describe '#pipeline' do let(:project) { create :project } let(:pipeline) { create :ci_pipeline, project: project, ref: 'master' } @@ -588,7 +588,7 @@ describe Project, models: true do end end - describe :builds_enabled do + describe '#builds_enabled' do let(:project) { create :project } before { project.builds_enabled = true } @@ -690,7 +690,7 @@ describe Project, models: true do end end - describe :any_runners do + describe '#any_runners' do let(:project) { create(:empty_project, shared_runners_enabled: shared_runners_enabled) } let(:specific_runner) { create(:ci_runner) } let(:shared_runner) { create(:ci_runner, :shared) } -- cgit v1.2.1 From 5d742a308c7b2e79a4da4c209e6c0f401d9921a6 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 12 Jul 2016 10:55:18 +0000 Subject: Improve wording in UI guide --- doc/development/ui_guide.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/development/ui_guide.md b/doc/development/ui_guide.md index 3c44affdd76..8d02c52c477 100644 --- a/doc/development/ui_guide.md +++ b/doc/development/ui_guide.md @@ -27,7 +27,7 @@ We try to keep the amount of tabs in the header navigation between 5 and 10 so t tab should represent separate functionality. Everything related to the issue tracker should be under the 'Issues' tab while everything related to the wiki should be under 'Wiki' tab and so on and so forth. -When add new tab to the header don't use more than 2 words for text in the link. +When adding a new tab to the header don't use more than 2 words for text in the link. We want to keep links short and easy to remember and fit all of them in the small screen. ## Mobile screen size -- cgit v1.2.1 From 3d2c540db6bb33f1a7be6c1ba375d4f604544862 Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Tue, 5 Jul 2016 17:21:13 +0200 Subject: Object renderer read_multi rendered entries from Cache --- CHANGELOG | 1 + lib/banzai.rb | 4 +++ lib/banzai/object_renderer.rb | 23 ++++++++------ lib/banzai/renderer.rb | 56 ++++++++++++++++++++++++++++++++- spec/lib/banzai/object_renderer_spec.rb | 25 ++++++++++----- 5 files changed, 91 insertions(+), 18 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 84bfc27b151..a5bd9e0daf2 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -51,6 +51,7 @@ v 8.10.0 (unreleased) - Don't instantiate a git tree on Projects show default view - Bump Rinku to 2.0.0 - Remove unused front-end variable -> default_issues_tracker + - ObjectRenderer retrieve renderer content using Rails.cache.read_multi - Better caching of git calls on ProjectsController#show. - Avoid to retrieve MR closes_issues as much as possible. - Add API endpoint for a group issues !4520 (mahcsig) diff --git a/lib/banzai.rb b/lib/banzai.rb index 093382261ae..9ebe379f454 100644 --- a/lib/banzai.rb +++ b/lib/banzai.rb @@ -3,6 +3,10 @@ module Banzai Renderer.render(text, context) end + def self.cache_collection_render(texts_and_contexts) + Renderer.cache_collection_render(texts_and_contexts) + end + def self.render_result(text, context = {}) Renderer.render_result(text, context) end diff --git a/lib/banzai/object_renderer.rb b/lib/banzai/object_renderer.rb index dc83b87a6c1..9aef807c152 100644 --- a/lib/banzai/object_renderer.rb +++ b/lib/banzai/object_renderer.rb @@ -39,9 +39,7 @@ module Banzai # Renders the attribute of every given object. def render_objects(objects, attribute) - objects.map do |object| - render_attribute(object, attribute) - end + render_attributes(objects, attribute) end # Redacts the list of documents. @@ -64,16 +62,21 @@ module Banzai context end - # Renders the attribute of an object. + # Renders the attributes of a set of objects. # - # Returns a `Nokogiri::HTML::Document`. - def render_attribute(object, attribute) - context = context_for(object, attribute) + # Returns an Array of `Nokogiri::HTML::Document`. + def render_attributes(objects, attribute) + strings_and_contexts = objects.map do |object| + context = context_for(object, attribute) + + string = object.__send__(attribute) - string = object.__send__(attribute) - html = Banzai.render(string, context) + { text: string, context: context } + end - Banzai::Pipeline[:relative_link].to_document(html, context) + Banzai.cache_collection_render(strings_and_contexts).each_with_index.map do |html, index| + Banzai::Pipeline[:relative_link].to_document(html, strings_and_contexts[index][:context]) + end end def base_context diff --git a/lib/banzai/renderer.rb b/lib/banzai/renderer.rb index 6718acdef7e..ea10b06439b 100644 --- a/lib/banzai/renderer.rb +++ b/lib/banzai/renderer.rb @@ -10,7 +10,7 @@ module Banzai # requiring XHTML, such as Atom feeds, need to call `post_process` on the # result, providing the appropriate `pipeline` option. # - # markdown - Markdown String + # text - Markdown String # context - Hash of context options passed to our HTML Pipeline # # Returns an HTML-safe String @@ -29,6 +29,52 @@ module Banzai end end + # Perform multiple render from an Array of Markdown String into an + # Array of HTML-safe String of HTML. + # + # As the rendered Markdown String can be already cached read all the data + # from the cache using Rails.cache.read_multi operation. If the Markdown String + # is not in the cache or it's not cacheable (no cache_key entry is provided in + # the context) the Markdown String is rendered and stored in the cache so the + # next render call gets the rendered HTML-safe String from the cache. + # + # For further explanation see #render method comments. + # + # texts_and_contexts - An Array of Hashes that contains the Markdown String (:text) + # an options passed to our HTML Pipeline (:context) + # + # If on the :context you specify a :cache_key entry will be used to retrieve it + # and cache the result of rendering the Markdown String. + # + # Returns an Array containing HTML-safe String instances. + # + # Example: + # texts_and_contexts + # => [{ text: '### Hello', + # context: { cache_key: [note, :note] } }] + def self.cache_collection_render(texts_and_contexts) + items_collection = texts_and_contexts.each_with_index do |item, index| + context = item[:context] + cache_key = full_cache_multi_key(context.delete(:cache_key), context[:pipeline]) + + item[:cache_key] = cache_key if cache_key + end + + cacheable_items, non_cacheable_items = items_collection.partition { |item| item.key?(:cache_key) } + items_in_cache = Rails.cache.read_multi(*cacheable_items.map { |item| item[:cache_key] }) + items_not_in_cache = cacheable_items.reject do |item| + item[:rendered] = items_in_cache[item[:cache_key]] + items_in_cache.key?(item[:cache_key]) + end + + (items_not_in_cache + non_cacheable_items).each do |item| + item[:rendered] = render(item[:text], item[:context]) + Rails.cache.write(item[:cache_key], item[:rendered]) if item[:cache_key] + end + + items_collection.map { |item| item[:rendered] } + end + def self.render_result(text, context = {}) text = Pipeline[:pre_process].to_html(text, context) if text @@ -78,5 +124,13 @@ module Banzai return unless cache_key ["banzai", *cache_key, pipeline_name || :full] end + + # To map Rails.cache.read_multi results we need to know the Rails.cache.expanded_key. + # Other option will be to generate stringified keys on our side and don't delegate to Rails.cache.expanded_key + # method. + def self.full_cache_multi_key(cache_key, pipeline_name) + return unless cache_key + Rails.cache.send(:expanded_key, full_cache_key(cache_key, pipeline_name)) + end end end diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb index d99175967af..cf6cdd33ebb 100644 --- a/spec/lib/banzai/object_renderer_spec.rb +++ b/spec/lib/banzai/object_renderer_spec.rb @@ -29,7 +29,7 @@ describe Banzai::ObjectRenderer do renderer = described_class.new(project, user) - expect(renderer).to receive(:render_attribute).with(object, :note). + expect(renderer).to receive(:render_attributes).with([object], :note). and_call_original rendered = renderer.render_objects([object], :note) @@ -89,14 +89,25 @@ describe Banzai::ObjectRenderer do end end - describe '#render_attribute' do - it 'renders the attribute of an object' do - object = double(:doc, note: 'hello') + describe '#render_attributes' do + it 'renders the attribute of a list of objects' do + objects = [double(:doc, note: 'hello'), double(:doc, note: 'bye')] renderer = described_class.new(project, user, pipeline: :note) - doc = renderer.render_attribute(object, :note) - expect(doc).to be_an_instance_of(Nokogiri::HTML::DocumentFragment) - expect(doc.to_html).to eq('

hello

') + expect(Banzai).to receive(:cache_collection_render). + with([ + { text: 'hello', context: renderer.context_for(objects[0], :note) }, + { text: 'bye', context: renderer.context_for(objects[1], :note) } + ]). + and_call_original + + docs = renderer.render_attributes(objects, :note) + + expect(docs[0]).to be_an_instance_of(Nokogiri::HTML::DocumentFragment) + expect(docs[0].to_html).to eq('

hello

') + + expect(docs[1]).to be_an_instance_of(Nokogiri::HTML::DocumentFragment) + expect(docs[1].to_html).to eq('

bye

') end end -- cgit v1.2.1 From f1f9b5e2f0f7ab8515f263b79c09d9f98b0d302f Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Tue, 12 Jul 2016 14:29:26 +0200 Subject: Be explicit on merge request discussion variables --- CHANGELOG | 1 + .../projects/merge_requests_controller.rb | 25 +++++++++++++--------- spec/features/merge_requests/diffs_spec.rb | 25 ++++++++++++++++++++++ 3 files changed, 41 insertions(+), 10 deletions(-) create mode 100644 spec/features/merge_requests/diffs_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 84bfc27b151..6c9affcdf1d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -56,6 +56,7 @@ v 8.10.0 (unreleased) - Add API endpoint for a group issues !4520 (mahcsig) - Add Bugzilla integration !4930 (iamtjg) - Instrument Rinku usage + - Be explicit to define merge request discussion variables - Metrics for Rouge::Plugins::Redcarpet and Rouge::Formatters::HTMLGitlab - RailsCache metris now includes fetch_hit/fetch_miss and read_hit/read_miss info. - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 941d68cda17..df659bb8c3b 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -56,7 +56,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController def show respond_to do |format| - format.html + format.html { define_discussion_vars } format.json do render json: @merge_request @@ -82,7 +82,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @merge_request_diff = @merge_request.merge_request_diff respond_to do |format| - format.html + format.html { define_discussion_vars } format.json { render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } } end end @@ -108,7 +108,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController def commits respond_to do |format| - format.html { render 'show' } + format.html do + define_discussion_vars + + render 'show' + end format.json do # Get commits from repository # or from cache if already merged @@ -123,7 +127,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController def builds respond_to do |format| - format.html { render 'show' } + format.html do + define_discussion_vars + + render 'show' + end format.json { render json: { html: view_to_html_string('projects/merge_requests/show/_builds') } } end end @@ -353,14 +361,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController @merge_request.unlock_mr @merge_request.close end - - if request.format == :html || action_name == 'show' - define_show_html_vars - end end - # Discussion tab data is only required on html requests - def define_show_html_vars + # Discussion tab data is rendered on html responses of actions + # :show, :diff, :commits, :builds. but not when request the data through AJAX + def define_discussion_vars # Build a note object for comment form @note = @project.notes.new(noteable: @noteable) diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb new file mode 100644 index 00000000000..c9a0059645d --- /dev/null +++ b/spec/features/merge_requests/diffs_spec.rb @@ -0,0 +1,25 @@ +require 'spec_helper' + +feature 'Diffs URL', js: true, feature: true do + before do + login_as :admin + @merge_request = create(:merge_request) + @project = @merge_request.source_project + end + + context 'when visit with */* as accept header' do + before(:each) do + page.driver.add_header('Accept', '*/*') + end + + it 'renders the notes' do + create :note_on_merge_request, project: @project, noteable: @merge_request, note: 'Rebasing with master' + + visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) + + # Load notes and diff through AJAX + expect(page).to have_css('.note-text', visible: false, text: 'Rebasing with master') + expect(page).to have_css('.diffs.tab-pane.active') + end + end +end -- cgit v1.2.1 From 3dc6bf2b71f995a3b6ca40ebbf9abb5c11397b8b Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 8 Jul 2016 12:32:58 -0700 Subject: Expire the branch cache after `git gc` runs Due to a stale NFS cache, it's possible that a branch lookup fails while `git gc` is running and causes missing branches in merge requests. Possible workaround for #15392 --- CHANGELOG | 1 + app/services/projects/housekeeping_service.rb | 4 +--- app/workers/git_garbage_collect_worker.rb | 14 +++++++++++++ app/workers/gitlab_shell_one_shot_worker.rb | 10 --------- .../services/projects/housekeeping_service_spec.rb | 4 ++-- spec/workers/git_garbage_collect_worker_spec.rb | 24 ++++++++++++++++++++++ 6 files changed, 42 insertions(+), 15 deletions(-) create mode 100644 app/workers/git_garbage_collect_worker.rb delete mode 100644 app/workers/gitlab_shell_one_shot_worker.rb create mode 100644 spec/workers/git_garbage_collect_worker_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 84bfc27b151..d77d02f82c3 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,7 @@ v 8.10.0 (unreleased) - Expose {should,force}_remove_source_branch (Ben Boeckel) - Fix commit builds API, return all builds for all pipelines for given commit. !4849 - Replace Haml with Hamlit to make view rendering faster. !3666 + - Expire the branch cache after `git gc` runs - Refactor repository paths handling to allow multiple git mount points - Optimize system note visibility checking by memoizing the visible reference count !5070 - Add Application Setting to configure default Repository Path for new projects diff --git a/app/services/projects/housekeeping_service.rb b/app/services/projects/housekeeping_service.rb index 752c11d7ae6..c9ad710b7bf 100644 --- a/app/services/projects/housekeeping_service.rb +++ b/app/services/projects/housekeeping_service.rb @@ -7,8 +7,6 @@ # module Projects class HousekeepingService < BaseService - include Gitlab::ShellAdapter - LEASE_TIMEOUT = 3600 class LeaseTaken < StandardError @@ -24,7 +22,7 @@ module Projects def execute raise LeaseTaken unless try_obtain_lease - GitlabShellOneShotWorker.perform_async(:gc, @project.repository_storage_path, @project.path_with_namespace) + GitGarbageCollectWorker.perform_async(@project.id) ensure Gitlab::Metrics.measure(:reset_pushes_since_gc) do update_pushes_since_gc(0) diff --git a/app/workers/git_garbage_collect_worker.rb b/app/workers/git_garbage_collect_worker.rb new file mode 100644 index 00000000000..2fa3c838f55 --- /dev/null +++ b/app/workers/git_garbage_collect_worker.rb @@ -0,0 +1,14 @@ +class GitGarbageCollectWorker + include Sidekiq::Worker + include Gitlab::ShellAdapter + + sidekiq_options queue: :gitlab_shell, retry: false + + def perform(project_id) + project = Project.find(project_id) + + gitlab_shell.gc(project.repository_storage_path, project.path_with_namespace) + # Expire the branch cache in case garbage collection caused a ref lookup to fail + project.repository.after_create_branch + end +end diff --git a/app/workers/gitlab_shell_one_shot_worker.rb b/app/workers/gitlab_shell_one_shot_worker.rb deleted file mode 100644 index 4ddbcf574d5..00000000000 --- a/app/workers/gitlab_shell_one_shot_worker.rb +++ /dev/null @@ -1,10 +0,0 @@ -class GitlabShellOneShotWorker - include Sidekiq::Worker - include Gitlab::ShellAdapter - - sidekiq_options queue: :gitlab_shell, retry: false - - def perform(action, *arg) - gitlab_shell.send(action, *arg) - end -end diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb index bd4dc6a0f79..7ab95e042ce 100644 --- a/spec/services/projects/housekeeping_service_spec.rb +++ b/spec/services/projects/housekeeping_service_spec.rb @@ -12,7 +12,7 @@ describe Projects::HousekeepingService do it 'enqueues a sidekiq job' do expect(subject).to receive(:try_obtain_lease).and_return(true) - expect(GitlabShellOneShotWorker).to receive(:perform_async).with(:gc, project.repository_storage_path, project.path_with_namespace) + expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id) subject.execute expect(project.pushes_since_gc).to eq(0) @@ -20,7 +20,7 @@ describe Projects::HousekeepingService do it 'does not enqueue a job when no lease can be obtained' do expect(subject).to receive(:try_obtain_lease).and_return(false) - expect(GitlabShellOneShotWorker).not_to receive(:perform_async) + expect(GitGarbageCollectWorker).not_to receive(:perform_async) expect { subject.execute }.to raise_error(Projects::HousekeepingService::LeaseTaken) expect(project.pushes_since_gc).to eq(0) diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb new file mode 100644 index 00000000000..a9cce8b8b59 --- /dev/null +++ b/spec/workers/git_garbage_collect_worker_spec.rb @@ -0,0 +1,24 @@ +require 'spec_helper' + +describe GitGarbageCollectWorker do + let(:project) { create(:project) } + let(:shell) { Gitlab::Shell.new } + + subject { GitGarbageCollectWorker.new } + + before do + allow(subject).to receive(:gitlab_shell).and_return(shell) + end + + describe "#perform" do + it "runs `git gc`" do + expect(shell).to receive(:gc).with( + project.repository_storage_path, + project.path_with_namespace). + and_return(true) + expect_any_instance_of(Repository).to receive(:after_create_branch) + + subject.perform(project.id) + end + end +end -- cgit v1.2.1 From d6e43f1f76f63d0a9e7ffe2b2130105c86b7a9fa Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 12 Jul 2016 13:52:24 +0000 Subject: Update Access Tokens link title to match the text --- app/views/layouts/nav/_profile.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/layouts/nav/_profile.html.haml b/app/views/layouts/nav/_profile.html.haml index 6e3c9ab9778..6d514f669db 100644 --- a/app/views/layouts/nav/_profile.html.haml +++ b/app/views/layouts/nav/_profile.html.haml @@ -18,7 +18,7 @@ %span Applications = nav_link(controller: :personal_access_tokens) do - = link_to profile_personal_access_tokens_path, title: 'Personal Access Tokens' do + = link_to profile_personal_access_tokens_path, title: 'Access Tokens' do %span Access Tokens = nav_link(controller: :emails) do -- cgit v1.2.1 From 0736397e0849333053390a24aa3938cda45707cf Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Tue, 12 Jul 2016 11:58:06 +0200 Subject: Reset project pushes_since_gc when we enqueue the git gc call --- CHANGELOG | 1 + app/services/projects/housekeeping_service.rb | 24 +++++++------ .../services/projects/housekeeping_service_spec.rb | 42 +++++++++++++++++----- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index ee3ee4c37d6..2527290f0fd 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -75,6 +75,7 @@ v 8.10.0 (unreleased) - Fix 404 redirect after validation fails importing a GitLab project - Added setting to set new users by default as external !4545 (Dravere) - Add min value for project limit field on user's form !3622 (jastkand) + - Reset project pushes_since_gc when we enqueue the git gc call - Add reminder to not paste private SSH keys !4399 (Ingo Blechschmidt) - Remove duplicate `description` field in `MergeRequest` entities (Ben Boeckel) - Style of import project buttons were fixed in the new project page. !5183 (rdemirbay) diff --git a/app/services/projects/housekeeping_service.rb b/app/services/projects/housekeeping_service.rb index c9ad710b7bf..29b3981f49f 100644 --- a/app/services/projects/housekeeping_service.rb +++ b/app/services/projects/housekeeping_service.rb @@ -22,11 +22,7 @@ module Projects def execute raise LeaseTaken unless try_obtain_lease - GitGarbageCollectWorker.perform_async(@project.id) - ensure - Gitlab::Metrics.measure(:reset_pushes_since_gc) do - update_pushes_since_gc(0) - end + execute_gitlab_shell_gc end def needed? @@ -34,19 +30,27 @@ module Projects end def increment! - Gitlab::Metrics.measure(:increment_pushes_since_gc) do - update_pushes_since_gc(@project.pushes_since_gc + 1) + if Gitlab::ExclusiveLease.new("project_housekeeping:increment!:#{@project.id}", timeout: 60).try_obtain + Gitlab::Metrics.measure(:increment_pushes_since_gc) do + update_pushes_since_gc(@project.pushes_since_gc + 1) + end end end private - def update_pushes_since_gc(new_value) - if Gitlab::ExclusiveLease.new("project_housekeeping:update_pushes_since_gc:#{project.id}", timeout: 60).try_obtain - @project.update_column(:pushes_since_gc, new_value) + def execute_gitlab_shell_gc + GitGarbageCollectWorker.perform_async(@project.id) + ensure + Gitlab::Metrics.measure(:reset_pushes_since_gc) do + update_pushes_since_gc(0) end end + def update_pushes_since_gc(new_value) + @project.update_column(:pushes_since_gc, new_value) + end + def try_obtain_lease Gitlab::Metrics.measure(:obtain_housekeeping_lease) do lease = ::Gitlab::ExclusiveLease.new("project_housekeeping:#{@project.id}", timeout: LEASE_TIMEOUT) diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb index 7ab95e042ce..ad0d58672b3 100644 --- a/spec/services/projects/housekeeping_service_spec.rb +++ b/spec/services/projects/housekeeping_service_spec.rb @@ -15,15 +15,25 @@ describe Projects::HousekeepingService do expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id) subject.execute - expect(project.pushes_since_gc).to eq(0) + expect(project.reload.pushes_since_gc).to eq(0) end - it 'does not enqueue a job when no lease can be obtained' do - expect(subject).to receive(:try_obtain_lease).and_return(false) - expect(GitGarbageCollectWorker).not_to receive(:perform_async) + context 'when no lease can be obtained' do + before(:each) do + expect(subject).to receive(:try_obtain_lease).and_return(false) + end - expect { subject.execute }.to raise_error(Projects::HousekeepingService::LeaseTaken) - expect(project.pushes_since_gc).to eq(0) + it 'does not enqueue a job' do + expect(GitGarbageCollectWorker).not_to receive(:perform_async) + + expect { subject.execute }.to raise_error(Projects::HousekeepingService::LeaseTaken) + end + + it 'does not reset pushes_since_gc' do + expect do + expect { subject.execute }.to raise_error(Projects::HousekeepingService::LeaseTaken) + end.not_to change { project.pushes_since_gc }.from(3) + end end end @@ -39,10 +49,24 @@ describe Projects::HousekeepingService do end describe 'increment!' do + let(:lease_key) { "project_housekeeping:increment!:#{project.id}" } + it 'increments the pushes_since_gc counter' do - expect(project.pushes_since_gc).to eq(0) - subject.increment! - expect(project.pushes_since_gc).to eq(1) + lease = double(:lease, try_obtain: true) + expect(Gitlab::ExclusiveLease).to receive(:new).with(lease_key, anything).and_return(lease) + + expect do + subject.increment! + end.to change { project.pushes_since_gc }.from(0).to(1) + end + + it 'does not increment when no lease can be obtained' do + lease = double(:lease, try_obtain: false) + expect(Gitlab::ExclusiveLease).to receive(:new).with(lease_key, anything).and_return(lease) + + expect do + subject.increment! + end.not_to change { project.pushes_since_gc } end end end -- cgit v1.2.1 From 20d8de8f673e040282ad0811fb8af8fa97aa3e92 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 12 Jul 2016 22:17:00 +0800 Subject: We're not using them (and look at the typo) --- spec/spec_helper.rb | 1 - spec/support/relative_url.rb | 8 -------- 2 files changed, 9 deletions(-) delete mode 100644 spec/support/relative_url.rb diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 606da1b7605..3638dcbb2d3 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -33,7 +33,6 @@ RSpec.configure do |config| config.include LoginHelpers, type: :request config.include StubConfiguration config.include EmailHelpers - config.include RelativeUrl, type: feature config.include TestEnv config.include ActiveJob::TestHelper config.include StubGitlabCalls diff --git a/spec/support/relative_url.rb b/spec/support/relative_url.rb deleted file mode 100644 index 72e3ccce75b..00000000000 --- a/spec/support/relative_url.rb +++ /dev/null @@ -1,8 +0,0 @@ -# Fix route helpers in tests (e.g. root_path, ...) -module RelativeUrl - extend ActiveSupport::Concern - - included do - default_url_options[:script_name] = Rails.application.config.relative_url_root - end -end -- cgit v1.2.1 From 3d1a75781260d394a13a30e0d267bcd79f3015dd Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Tue, 12 Jul 2016 17:35:39 +0300 Subject: Small refactor and addition of CI permissions --- doc/permissions/permissions.md | 53 +++++++++++++++++++++++++++++++----------- 1 file changed, 40 insertions(+), 13 deletions(-) diff --git a/doc/permissions/permissions.md b/doc/permissions/permissions.md index 44f3f6d3b12..8fc10a13f91 100644 --- a/doc/permissions/permissions.md +++ b/doc/permissions/permissions.md @@ -1,19 +1,21 @@ # Permissions -Users have different abilities depending on the access level they have in a particular group or project. +Users have different abilities depending on the access level they have in a +particular group or project. If a user is both in a group's project and the +project itself, the highest permission level is used. -If a user is both in a project group and in the project itself, the highest permission level is used. +On public and internal projects the Guest role is not enforced. All users will +be able to create issues, leave comments, and pull or download the project code. -If a user is a GitLab administrator they receive all permissions. - -On public and internal projects the Guest role is not enforced. -All users will be able to create issues, leave comments, and pull or download the project code. +GitLab administrators receive all permissions. To add or import a user, you can follow the [project users and members documentation](../workflow/add-user/add-user.md). ## Project +The following table depicts the various user permission levels in a project. + | Action | Guest | Reporter | Developer | Master | Owner | |---------------------------------------|---------|------------|-------------|----------|--------| | Create new issue | ✓ | ✓ | ✓ | ✓ | ✓ | @@ -46,8 +48,8 @@ documentation](../workflow/add-user/add-user.md). | Add new team members | | | | ✓ | ✓ | | Push to protected branches | | | | ✓ | ✓ | | Enable/disable branch protection | | | | ✓ | ✓ | -| Turn on/off prot. branch push for devs| | | | ✓ | ✓ | -| Rewrite/remove git tags | | | | ✓ | ✓ | +| Turn on/off protected branch push for devs| | | | ✓ | ✓ | +| Rewrite/remove Git tags | | | | ✓ | ✓ | | Edit project | | | | ✓ | ✓ | | Add deploy keys to project | | | | ✓ | ✓ | | Configure project hooks | | | | ✓ | ✓ | @@ -66,10 +68,9 @@ documentation](../workflow/add-user/add-user.md). ## Group -In order for a group to appear as public and be browsable, it must contain at -least one public project. - -Any user can remove themselves from a group, unless they are the last Owner of the group. +Any user can remove themselves from a group, unless they are the last Owner of +the group. The following table depicts the various user permission levels in a +group. | Action | Guest | Reporter | Developer | Master | Owner | |-------------------------|-------|----------|-----------|--------|-------| @@ -101,4 +102,30 @@ to **Admin > Users** to create a new user or edit an existing one. There, you will find the option to flag the user as external. By default new users are not set as external users. This behavior can be changed -by an administrator under **Admin > Application Settings**. \ No newline at end of file +by an administrator under **Admin > Application Settings**. + +## GitLab CI + +GitLab CI permissions rely on the role the user has in GitLab. There are four +permission levels it total: + +- admin +- master +- developer +- guest/reporter + +The admin user can perform any action on GitLab CI in scope of the GitLab +instance and project. In addition, all admins can use the admin interface under +`/admin/runners`. + +| Action | Guest, Reporter | Developer | Master | Admin | +|---------------------------------------|-----------------|-------------|----------|--------| +| See commits and builds | ✓ | ✓ | ✓ | ✓ | +| Retry or cancel build | | ✓ | ✓ | ✓ | +| Remove project | | | ✓ | ✓ | +| Create project | | | ✓ | ✓ | +| Change project configuration | | | ✓ | ✓ | +| Add specific runners | | | ✓ | ✓ | +| Add shared runners | | | | ✓ | +| See events in the system | | | | ✓ | +| Admin interface | | | | ✓ | -- cgit v1.2.1 From 429ab4a0b7683dd2e855e0299907ef9061f1c654 Mon Sep 17 00:00:00 2001 From: Connor Shea Date: Mon, 11 Jul 2016 16:29:25 -0600 Subject: Remove the group avatar link. For group pages, e.g. https://gitlab.com/groups/gitlab-org, I see no reason to link to the avatar image itself when clicking on the avatar. If the user really wants to open the image, they can do so by right clicking and opening the image in a new tab. --- app/views/groups/show.html.haml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index a83eb7e88bb..eddeae98bc4 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -6,8 +6,7 @@ .cover-block.groups-cover-block %div{ class: container_class } - = link_to group_icon(@group), target: '_blank' do - = image_tag group_icon(@group), class: "avatar group-avatar s70" + = image_tag group_icon(@group), class: "avatar group-avatar s70" .group-info .cover-title %h1 -- cgit v1.2.1 From c6735f9f2068ba618165d8719405eda7899fa772 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Tue, 12 Jul 2016 17:49:03 +0300 Subject: Move permissions/permissions.md to user/permissions.md --- doc/permissions/permissions.md | 130 +--------------------------------------- doc/user/permissions.md | 131 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 129 deletions(-) create mode 100644 doc/user/permissions.md diff --git a/doc/permissions/permissions.md b/doc/permissions/permissions.md index 8fc10a13f91..78d67aeec78 100644 --- a/doc/permissions/permissions.md +++ b/doc/permissions/permissions.md @@ -1,131 +1,3 @@ # Permissions -Users have different abilities depending on the access level they have in a -particular group or project. If a user is both in a group's project and the -project itself, the highest permission level is used. - -On public and internal projects the Guest role is not enforced. All users will -be able to create issues, leave comments, and pull or download the project code. - -GitLab administrators receive all permissions. - -To add or import a user, you can follow the [project users and members -documentation](../workflow/add-user/add-user.md). - -## Project - -The following table depicts the various user permission levels in a project. - -| Action | Guest | Reporter | Developer | Master | Owner | -|---------------------------------------|---------|------------|-------------|----------|--------| -| Create new issue | ✓ | ✓ | ✓ | ✓ | ✓ | -| Leave comments | ✓ | ✓ | ✓ | ✓ | ✓ | -| See a list of builds | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | -| See a build log | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | -| Download and browse build artifacts | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | -| Pull project code | | ✓ | ✓ | ✓ | ✓ | -| Download project | | ✓ | ✓ | ✓ | ✓ | -| Create code snippets | | ✓ | ✓ | ✓ | ✓ | -| Manage issue tracker | | ✓ | ✓ | ✓ | ✓ | -| Manage labels | | ✓ | ✓ | ✓ | ✓ | -| See a commit status | | ✓ | ✓ | ✓ | ✓ | -| See a container registry | | ✓ | ✓ | ✓ | ✓ | -| See environments | | ✓ | ✓ | ✓ | ✓ | -| Manage merge requests | | | ✓ | ✓ | ✓ | -| Create new merge request | | | ✓ | ✓ | ✓ | -| Create new branches | | | ✓ | ✓ | ✓ | -| Push to non-protected branches | | | ✓ | ✓ | ✓ | -| Force push to non-protected branches | | | ✓ | ✓ | ✓ | -| Remove non-protected branches | | | ✓ | ✓ | ✓ | -| Add tags | | | ✓ | ✓ | ✓ | -| Write a wiki | | | ✓ | ✓ | ✓ | -| Cancel and retry builds | | | ✓ | ✓ | ✓ | -| Create or update commit status | | | ✓ | ✓ | ✓ | -| Update a container registry | | | ✓ | ✓ | ✓ | -| Remove a container registry image | | | ✓ | ✓ | ✓ | -| Create new environments | | | ✓ | ✓ | ✓ | -| Create new milestones | | | | ✓ | ✓ | -| Add new team members | | | | ✓ | ✓ | -| Push to protected branches | | | | ✓ | ✓ | -| Enable/disable branch protection | | | | ✓ | ✓ | -| Turn on/off protected branch push for devs| | | | ✓ | ✓ | -| Rewrite/remove Git tags | | | | ✓ | ✓ | -| Edit project | | | | ✓ | ✓ | -| Add deploy keys to project | | | | ✓ | ✓ | -| Configure project hooks | | | | ✓ | ✓ | -| Manage runners | | | | ✓ | ✓ | -| Manage build triggers | | | | ✓ | ✓ | -| Manage variables | | | | ✓ | ✓ | -| Delete environments | | | | ✓ | ✓ | -| Switch visibility level | | | | | ✓ | -| Transfer project to another namespace | | | | | ✓ | -| Remove project | | | | | ✓ | -| Force push to protected branches [^2] | | | | | | -| Remove protected branches [^2] | | | | | | - -[^1]: If **Allow guest to access builds** is enabled in CI settings -[^2]: Not allowed for Guest, Reporter, Developer, Master, or Owner - -## Group - -Any user can remove themselves from a group, unless they are the last Owner of -the group. The following table depicts the various user permission levels in a -group. - -| Action | Guest | Reporter | Developer | Master | Owner | -|-------------------------|-------|----------|-----------|--------|-------| -| Browse group | ✓ | ✓ | ✓ | ✓ | ✓ | -| Edit group | | | | | ✓ | -| Create project in group | | | | ✓ | ✓ | -| Manage group members | | | | | ✓ | -| Remove group | | | | | ✓ | - -## External Users - -In cases where it is desired that a user has access only to some internal or -private projects, there is the option of creating **External Users**. This -feature may be useful when for example a contractor is working on a given -project and should only have access to that project. - -External users can only access projects to which they are explicitly granted -access, thus hiding all other internal or private ones from them. Access can be -granted by adding the user as member to the project or group. - -They will, like usual users, receive a role in the project or group with all -the abilities that are mentioned in the table above. They cannot however create -groups or projects, and they have the same access as logged out users in all -other cases. - -An administrator can flag a user as external [through the API](../api/users.md) -or by checking the checkbox on the admin panel. As an administrator, navigate -to **Admin > Users** to create a new user or edit an existing one. There, you -will find the option to flag the user as external. - -By default new users are not set as external users. This behavior can be changed -by an administrator under **Admin > Application Settings**. - -## GitLab CI - -GitLab CI permissions rely on the role the user has in GitLab. There are four -permission levels it total: - -- admin -- master -- developer -- guest/reporter - -The admin user can perform any action on GitLab CI in scope of the GitLab -instance and project. In addition, all admins can use the admin interface under -`/admin/runners`. - -| Action | Guest, Reporter | Developer | Master | Admin | -|---------------------------------------|-----------------|-------------|----------|--------| -| See commits and builds | ✓ | ✓ | ✓ | ✓ | -| Retry or cancel build | | ✓ | ✓ | ✓ | -| Remove project | | | ✓ | ✓ | -| Create project | | | ✓ | ✓ | -| Change project configuration | | | ✓ | ✓ | -| Add specific runners | | | ✓ | ✓ | -| Add shared runners | | | | ✓ | -| See events in the system | | | | ✓ | -| Admin interface | | | | ✓ | +This document was moved to [user/permissions.md](../user/permissions.md). diff --git a/doc/user/permissions.md b/doc/user/permissions.md new file mode 100644 index 00000000000..8fc10a13f91 --- /dev/null +++ b/doc/user/permissions.md @@ -0,0 +1,131 @@ +# Permissions + +Users have different abilities depending on the access level they have in a +particular group or project. If a user is both in a group's project and the +project itself, the highest permission level is used. + +On public and internal projects the Guest role is not enforced. All users will +be able to create issues, leave comments, and pull or download the project code. + +GitLab administrators receive all permissions. + +To add or import a user, you can follow the [project users and members +documentation](../workflow/add-user/add-user.md). + +## Project + +The following table depicts the various user permission levels in a project. + +| Action | Guest | Reporter | Developer | Master | Owner | +|---------------------------------------|---------|------------|-------------|----------|--------| +| Create new issue | ✓ | ✓ | ✓ | ✓ | ✓ | +| Leave comments | ✓ | ✓ | ✓ | ✓ | ✓ | +| See a list of builds | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | +| See a build log | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | +| Download and browse build artifacts | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | +| Pull project code | | ✓ | ✓ | ✓ | ✓ | +| Download project | | ✓ | ✓ | ✓ | ✓ | +| Create code snippets | | ✓ | ✓ | ✓ | ✓ | +| Manage issue tracker | | ✓ | ✓ | ✓ | ✓ | +| Manage labels | | ✓ | ✓ | ✓ | ✓ | +| See a commit status | | ✓ | ✓ | ✓ | ✓ | +| See a container registry | | ✓ | ✓ | ✓ | ✓ | +| See environments | | ✓ | ✓ | ✓ | ✓ | +| Manage merge requests | | | ✓ | ✓ | ✓ | +| Create new merge request | | | ✓ | ✓ | ✓ | +| Create new branches | | | ✓ | ✓ | ✓ | +| Push to non-protected branches | | | ✓ | ✓ | ✓ | +| Force push to non-protected branches | | | ✓ | ✓ | ✓ | +| Remove non-protected branches | | | ✓ | ✓ | ✓ | +| Add tags | | | ✓ | ✓ | ✓ | +| Write a wiki | | | ✓ | ✓ | ✓ | +| Cancel and retry builds | | | ✓ | ✓ | ✓ | +| Create or update commit status | | | ✓ | ✓ | ✓ | +| Update a container registry | | | ✓ | ✓ | ✓ | +| Remove a container registry image | | | ✓ | ✓ | ✓ | +| Create new environments | | | ✓ | ✓ | ✓ | +| Create new milestones | | | | ✓ | ✓ | +| Add new team members | | | | ✓ | ✓ | +| Push to protected branches | | | | ✓ | ✓ | +| Enable/disable branch protection | | | | ✓ | ✓ | +| Turn on/off protected branch push for devs| | | | ✓ | ✓ | +| Rewrite/remove Git tags | | | | ✓ | ✓ | +| Edit project | | | | ✓ | ✓ | +| Add deploy keys to project | | | | ✓ | ✓ | +| Configure project hooks | | | | ✓ | ✓ | +| Manage runners | | | | ✓ | ✓ | +| Manage build triggers | | | | ✓ | ✓ | +| Manage variables | | | | ✓ | ✓ | +| Delete environments | | | | ✓ | ✓ | +| Switch visibility level | | | | | ✓ | +| Transfer project to another namespace | | | | | ✓ | +| Remove project | | | | | ✓ | +| Force push to protected branches [^2] | | | | | | +| Remove protected branches [^2] | | | | | | + +[^1]: If **Allow guest to access builds** is enabled in CI settings +[^2]: Not allowed for Guest, Reporter, Developer, Master, or Owner + +## Group + +Any user can remove themselves from a group, unless they are the last Owner of +the group. The following table depicts the various user permission levels in a +group. + +| Action | Guest | Reporter | Developer | Master | Owner | +|-------------------------|-------|----------|-----------|--------|-------| +| Browse group | ✓ | ✓ | ✓ | ✓ | ✓ | +| Edit group | | | | | ✓ | +| Create project in group | | | | ✓ | ✓ | +| Manage group members | | | | | ✓ | +| Remove group | | | | | ✓ | + +## External Users + +In cases where it is desired that a user has access only to some internal or +private projects, there is the option of creating **External Users**. This +feature may be useful when for example a contractor is working on a given +project and should only have access to that project. + +External users can only access projects to which they are explicitly granted +access, thus hiding all other internal or private ones from them. Access can be +granted by adding the user as member to the project or group. + +They will, like usual users, receive a role in the project or group with all +the abilities that are mentioned in the table above. They cannot however create +groups or projects, and they have the same access as logged out users in all +other cases. + +An administrator can flag a user as external [through the API](../api/users.md) +or by checking the checkbox on the admin panel. As an administrator, navigate +to **Admin > Users** to create a new user or edit an existing one. There, you +will find the option to flag the user as external. + +By default new users are not set as external users. This behavior can be changed +by an administrator under **Admin > Application Settings**. + +## GitLab CI + +GitLab CI permissions rely on the role the user has in GitLab. There are four +permission levels it total: + +- admin +- master +- developer +- guest/reporter + +The admin user can perform any action on GitLab CI in scope of the GitLab +instance and project. In addition, all admins can use the admin interface under +`/admin/runners`. + +| Action | Guest, Reporter | Developer | Master | Admin | +|---------------------------------------|-----------------|-------------|----------|--------| +| See commits and builds | ✓ | ✓ | ✓ | ✓ | +| Retry or cancel build | | ✓ | ✓ | ✓ | +| Remove project | | | ✓ | ✓ | +| Create project | | | ✓ | ✓ | +| Change project configuration | | | ✓ | ✓ | +| Add specific runners | | | ✓ | ✓ | +| Add shared runners | | | | ✓ | +| See events in the system | | | | ✓ | +| Admin interface | | | | ✓ | -- cgit v1.2.1 From 523168c8bf29e0219620acce96ea07cadfdfcb13 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Tue, 12 Jul 2016 17:52:48 +0300 Subject: Move CI permissions to permissions.md --- doc/ci/README.md | 2 +- doc/ci/permissions/README.md | 23 +---------------------- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/doc/ci/README.md b/doc/ci/README.md index a9d407528e8..0833027f91d 100644 --- a/doc/ci/README.md +++ b/doc/ci/README.md @@ -15,6 +15,6 @@ - [Use SSH keys in your build environment](ssh_keys/README.md) - [Trigger builds through the API](triggers/README.md) - [Build artifacts](build_artifacts/README.md) -- [User permissions](permissions/README.md) +- [User permissions](../user/permissions.md#gitlab-ci) - [API](../api/ci/README.md) - [CI services (linked docker containers)](services/README.md) diff --git a/doc/ci/permissions/README.md b/doc/ci/permissions/README.md index d77061c14cd..42eb59f84c8 100644 --- a/doc/ci/permissions/README.md +++ b/doc/ci/permissions/README.md @@ -1,24 +1,3 @@ # Users Permissions -GitLab CI relies on user's role on the GitLab. There are three permissions levels on GitLab CI: admin, master, developer, other. - -Admin user can perform any actions on GitLab CI in scope of instance and project. Also user with admin permission can use admin interface. - - - - -| Action | Guest, Reporter | Developer | Master | Admin | -|---------------------------------------|-----------------|-------------|----------|--------| -| See commits and builds | ✓ | ✓ | ✓ | ✓ | -| Retry or cancel build | | ✓ | ✓ | ✓ | -| Remove project | | | ✓ | ✓ | -| Create project | | | ✓ | ✓ | -| Change project configuration | | | ✓ | ✓ | -| Add specific runners | | | ✓ | ✓ | -| Add shared runners | | | | ✓ | -| See events in the system | | | | ✓ | -| Admin interface | | | | ✓ | - - - - +This document was moved to [user/permissions.md](../../user/permissions.md#gitlab-ci). -- cgit v1.2.1 From aa8a61cca9b2346212c19d365c2e60841fea6212 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Tue, 12 Jul 2016 17:53:54 +0300 Subject: Change permissions URL in main readme --- doc/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/README.md b/doc/README.md index cf7a828d91e..cc0b6e0c1e5 100644 --- a/doc/README.md +++ b/doc/README.md @@ -11,7 +11,7 @@ - [Importing and exporting projects between instances](user/project/settings/import_export.md). - [Markdown](markdown/markdown.md) GitLab's advanced formatting system. - [Migrating from SVN](workflow/importing/migrating_from_svn.md) Convert a SVN repository to Git and GitLab. -- [Permissions](permissions/permissions.md) Learn what each role in a project (external/guest/reporter/developer/master/owner) can do. +- [Permissions](user/permissions.md) Learn what each role in a project (external/guest/reporter/developer/master/owner) can do. - [Profile Settings](profile/README.md) - [Project Services](project_services/project_services.md) Integrate a project with external services, such as CI and chat. - [Public access](public_access/public_access.md) Learn how you can allow public and internal access to projects. -- cgit v1.2.1 From 801453297d29600d7c959e48e8b7020e4348c50c Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Tue, 12 Jul 2016 09:57:05 -0500 Subject: Remove icons from sidebar; remove darker theme color from pin & toggle buttons --- app/assets/stylesheets/framework/gitlab-theme.scss | 1 - app/assets/stylesheets/framework/sidebar.scss | 2 +- app/views/layouts/nav/_dashboard.html.haml | 20 -------------------- 3 files changed, 1 insertion(+), 22 deletions(-) diff --git a/app/assets/stylesheets/framework/gitlab-theme.scss b/app/assets/stylesheets/framework/gitlab-theme.scss index d1ff63bd099..3673b81f183 100644 --- a/app/assets/stylesheets/framework/gitlab-theme.scss +++ b/app/assets/stylesheets/framework/gitlab-theme.scss @@ -11,7 +11,6 @@ .toggle-nav-collapse, .pin-nav-btn { color: $color-light; - background: $color; &:hover { color: $white-light; diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 1a2220f3b40..9aefc158d6e 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -76,7 +76,7 @@ } a { - padding: 7px 15px 7px 12px; + padding: 7px 15px; font-size: $gl-font-size; line-height: 24px; display: block; diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 52e41b1a857..21668698814 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -1,64 +1,44 @@ %ul.nav.nav-sidebar = nav_link(path: ['root#index', 'projects#trending', 'projects#starred', 'dashboard/projects#index'], html_options: {class: "#{project_tab_class} home"}) do = link_to dashboard_projects_path, title: 'Projects', class: 'dashboard-shortcuts-projects' do - .icon-container - = navbar_icon('project') %span Projects = nav_link(controller: :todos) do = link_to dashboard_todos_path, title: 'Todos' do - .icon-container - = icon('bell fw') %span Todos %span.count= number_with_delimiter(todos_pending_count) = nav_link(path: 'dashboard#activity') do = link_to activity_dashboard_path, class: 'dashboard-shortcuts-activity', title: 'Activity' do - .icon-container - = navbar_icon('activity') %span Activity = nav_link(controller: [:groups, 'groups/milestones', 'groups/group_members']) do = link_to dashboard_groups_path, title: 'Groups' do - .icon-container - = navbar_icon('group') %span Groups = nav_link(controller: 'dashboard/milestones') do = link_to dashboard_milestones_path, title: 'Milestones' do - .icon-container - = navbar_icon('milestones') %span Milestones = nav_link(path: 'dashboard#issues') do = link_to assigned_issues_dashboard_path, title: 'Issues', class: 'dashboard-shortcuts-issues' do - .icon-container - = navbar_icon('issues') %span Issues %span.count= number_with_delimiter(current_user.assigned_issues.opened.count) = nav_link(path: 'dashboard#merge_requests') do = link_to assigned_mrs_dashboard_path, title: 'Merge Requests', class: 'dashboard-shortcuts-merge_requests' do - .icon-container - = navbar_icon('mr') %span Merge Requests %span.count= number_with_delimiter(current_user.assigned_merge_requests.opened.count) = nav_link(controller: :snippets) do = link_to dashboard_snippets_path, title: 'Snippets' do - .icon-container - = icon('clipboard fw') %span Snippets = nav_link(controller: :help) do = link_to help_path, title: 'Help' do - .icon-container - = icon('question-circle fw') %span Help = nav_link(html_options: {class: profile_tab_class}) do = link_to profile_path, title: 'Profile Settings', data: {placement: 'bottom'} do - .icon-container - = icon('user fw') %span Profile Settings -- cgit v1.2.1 From f031caf3aebb9550ec23fcdf7ec5e4080dfb5ce7 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Tue, 12 Jul 2016 18:03:29 +0300 Subject: Change URLs to new permissions.md location --- doc/integration/saml.md | 4 ++-- doc/public_access/public_access.md | 4 ++-- doc/workflow/add-user/add-user.md | 2 +- doc/workflow/forking_workflow.md | 2 +- doc/workflow/protected_branches.md | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/doc/integration/saml.md b/doc/integration/saml.md index 8a7205caaa4..f3b2a288776 100644 --- a/doc/integration/saml.md +++ b/doc/integration/saml.md @@ -138,7 +138,7 @@ This setting is only available on GitLab 8.7 and above. SAML login includes support for external groups. You can define in the SAML settings which groups, to which your users belong in your IdP, you wish to be -marked as [external](../permissions/permissions.md). +marked as [external](../user/permissions.md). ### Requirements @@ -306,4 +306,4 @@ For this you need take the following into account: validators are optional Make sure that one of the above described scenarios is valid, or the requests will -fail with one of the mentioned errors. \ No newline at end of file +fail with one of the mentioned errors. diff --git a/doc/public_access/public_access.md b/doc/public_access/public_access.md index 9a5c5a5c92a..a3921f1b89f 100644 --- a/doc/public_access/public_access.md +++ b/doc/public_access/public_access.md @@ -17,7 +17,7 @@ Public projects can be cloned **without any** authentication. They will also be listed on the public access directory (`/public`). -**Any logged in user** will have [Guest](../permissions/permissions.md) +**Any logged in user** will have [Guest](../user/permissions.md) permissions on the repository. ### Internal projects @@ -27,7 +27,7 @@ Internal projects can be cloned by any logged in user. They will also be listed on the public access directory (`/public`) for logged in users. -Any logged in user will have [Guest](../permissions/permissions.md) permissions +Any logged in user will have [Guest](../user/permissions.md) permissions on the repository. ### How to change project visibility diff --git a/doc/workflow/add-user/add-user.md b/doc/workflow/add-user/add-user.md index 4b551130255..0537ce0bcd4 100644 --- a/doc/workflow/add-user/add-user.md +++ b/doc/workflow/add-user/add-user.md @@ -23,7 +23,7 @@ want to add. --- -Select the user and the [permission level](../../permissions/permissions.md) +Select the user and the [permission level](../../user/permissions.md) that you'd like to give the user. Note that you can select more than one user. ![Give user permissions](img/add_user_give_permissions.png) diff --git a/doc/workflow/forking_workflow.md b/doc/workflow/forking_workflow.md index 217a4a4012f..733d079bd4a 100644 --- a/doc/workflow/forking_workflow.md +++ b/doc/workflow/forking_workflow.md @@ -38,7 +38,7 @@ Forking a project is in most cases a two-step process. --- After the forking is done, you can start working on the newly created -repository. There, you will have full [Owner](../permissions/permissions.md) +repository. There, you will have full [Owner](../user/permissions.md) access, so you can set it up as you please. ## Merging upstream diff --git a/doc/workflow/protected_branches.md b/doc/workflow/protected_branches.md index 67adfc2f43a..5c1c7b47c8a 100644 --- a/doc/workflow/protected_branches.md +++ b/doc/workflow/protected_branches.md @@ -12,7 +12,7 @@ A protected branch does three simple things: You can make any branch a protected branch. GitLab makes the master branch a protected branch by default. -To protect a branch, user needs to have at least a Master permission level, see [permissions document](../permissions/permissions.md). +To protect a branch, user needs to have at least a Master permission level, see [permissions document](../user/permissions.md). ![protected branches page](protected_branches/protected_branches1.png) -- cgit v1.2.1 From 38a58978aa7969908ef31f2eac99138da2be869b Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 11 Jul 2016 16:15:59 -0300 Subject: Fix GItHub client requests when rate limit is disabled --- lib/gitlab/github_import/client.rb | 5 +++++ spec/lib/gitlab/github_import/client_spec.rb | 9 +++++++++ 2 files changed, 14 insertions(+) diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb index 043f10d96a9..f57f5b74706 100644 --- a/lib/gitlab/github_import/client.rb +++ b/lib/gitlab/github_import/client.rb @@ -78,6 +78,11 @@ module Gitlab def rate_limit api.rate_limit! + # GitHub Rate Limit API returns 404 when the rate limit is + # disabled. In this case we just want to return gracefully + # instead of spitting out an error. + rescue Octokit::NotFound + OpenStruct.new(remaining: GITHUB_SAFE_REMAINING_REQUESTS + 1) end def rate_limit_exceed? diff --git a/spec/lib/gitlab/github_import/client_spec.rb b/spec/lib/gitlab/github_import/client_spec.rb index 3b023a35446..efce10dbf15 100644 --- a/spec/lib/gitlab/github_import/client_spec.rb +++ b/spec/lib/gitlab/github_import/client_spec.rb @@ -61,4 +61,13 @@ describe Gitlab::GithubImport::Client, lib: true do expect(client.api.api_endpoint).to eq 'https://github.company.com/' end end + + context 'when rate limit is disabled' do + it 'does not raise error' do + stub_request(:get, /api.github.com/) + allow(client.api).to receive(:rate_limit!).and_raise(Octokit::NotFound) + + expect { client.issues }.not_to raise_error + end + end end -- cgit v1.2.1 From cfd584b2835bc1e15eeac29433593da5f16266f3 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 11 Jul 2016 16:16:08 -0300 Subject: Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 2527290f0fd..be8dfb62245 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -79,6 +79,7 @@ v 8.10.0 (unreleased) - Add reminder to not paste private SSH keys !4399 (Ingo Blechschmidt) - Remove duplicate `description` field in `MergeRequest` entities (Ben Boeckel) - Style of import project buttons were fixed in the new project page. !5183 (rdemirbay) + - Fix GitHub client requests when rate limit is disabled v 8.9.6 (unreleased) - Fix importing of events under notes for GitLab projects -- cgit v1.2.1 From 3ddcd0d699153363359faf28ab9f53cfd46a1cf9 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 11 Jul 2016 16:23:23 -0300 Subject: Remove unnecessary context from GitHub client spec --- spec/lib/gitlab/github_import/client_spec.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/lib/gitlab/github_import/client_spec.rb b/spec/lib/gitlab/github_import/client_spec.rb index efce10dbf15..613c47d55f1 100644 --- a/spec/lib/gitlab/github_import/client_spec.rb +++ b/spec/lib/gitlab/github_import/client_spec.rb @@ -62,12 +62,10 @@ describe Gitlab::GithubImport::Client, lib: true do end end - context 'when rate limit is disabled' do - it 'does not raise error' do - stub_request(:get, /api.github.com/) - allow(client.api).to receive(:rate_limit!).and_raise(Octokit::NotFound) + it 'does not raise error when rate limit is disabled' do + stub_request(:get, /api.github.com/) + allow(client.api).to receive(:rate_limit!).and_raise(Octokit::NotFound) - expect { client.issues }.not_to raise_error - end + expect { client.issues }.not_to raise_error end end -- cgit v1.2.1 From be9262dd951e056e832ec51f8b74a8cb7ce866b4 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 11 Jul 2016 18:53:39 -0300 Subject: Checks if rate limit is enabled instead of stubbing response --- lib/gitlab/github_import/client.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb index f57f5b74706..f2e220fcb3e 100644 --- a/lib/gitlab/github_import/client.rb +++ b/lib/gitlab/github_import/client.rb @@ -82,11 +82,15 @@ module Gitlab # disabled. In this case we just want to return gracefully # instead of spitting out an error. rescue Octokit::NotFound - OpenStruct.new(remaining: GITHUB_SAFE_REMAINING_REQUESTS + 1) + nil + end + + def has_rate_limit? + rate_limit.present? end def rate_limit_exceed? - rate_limit.remaining <= GITHUB_SAFE_REMAINING_REQUESTS + has_rate_limit? && rate_limit.remaining <= GITHUB_SAFE_REMAINING_REQUESTS end def rate_limit_sleep_time -- cgit v1.2.1 From 78a5de99e9686bce11ba386e5d59c3e8085e40be Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 12 Jul 2016 10:44:46 -0300 Subject: Memoize response from `has_rate_limit?` to avoid extra API call --- lib/gitlab/github_import/client.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb index f2e220fcb3e..084e514492c 100644 --- a/lib/gitlab/github_import/client.rb +++ b/lib/gitlab/github_import/client.rb @@ -86,7 +86,9 @@ module Gitlab end def has_rate_limit? - rate_limit.present? + return @has_rate_limit if defined?(@has_rate_limit) + + @has_rate_limit = rate_limit.present? end def rate_limit_exceed? -- cgit v1.2.1 From 461feb3ddf75eb175a138fc37385ad80b0bbda9f Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Tue, 12 Jul 2016 18:16:38 +0300 Subject: Add 'Accept merge request' to permissions table --- doc/user/permissions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 8fc10a13f91..66542861781 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -31,7 +31,7 @@ The following table depicts the various user permission levels in a project. | See a commit status | | ✓ | ✓ | ✓ | ✓ | | See a container registry | | ✓ | ✓ | ✓ | ✓ | | See environments | | ✓ | ✓ | ✓ | ✓ | -| Manage merge requests | | | ✓ | ✓ | ✓ | +| Manage/Accept merge requests | | | ✓ | ✓ | ✓ | | Create new merge request | | | ✓ | ✓ | ✓ | | Create new branches | | | ✓ | ✓ | ✓ | | Push to non-protected branches | | | ✓ | ✓ | ✓ | -- cgit v1.2.1 From 97606e7395be10de77a11ed2f0b03ebb14d1d9f6 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Tue, 12 Jul 2016 10:18:44 -0500 Subject: Remove toggle partial; Move pin link to top of side nav --- app/assets/stylesheets/framework/sidebar.scss | 57 +++++++++++++------------ app/assets/stylesheets/framework/variables.scss | 1 + app/views/layouts/_collapse_button.html.haml | 3 -- app/views/layouts/_page.html.haml | 12 ++++-- 4 files changed, 38 insertions(+), 35 deletions(-) delete mode 100644 app/views/layouts/_collapse_button.html.haml diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 9aefc158d6e..99833ef842a 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -17,7 +17,7 @@ height: 100%; overflow: hidden; transition: width $sidebar-transition-duration; - @include box-shadow(2px 0 16px 0 #bbb); + @include box-shadow(2px 0 16px 0 $box-shadow-gray); } } @@ -76,7 +76,7 @@ } a { - padding: 7px 15px; + padding: 7px 16px; font-size: $gl-font-size; line-height: 24px; display: block; @@ -108,7 +108,7 @@ } } -.toggle-nav-collapse { +.sidebar-action-buttons { width: $sidebar_width; position: absolute; top: 0; @@ -117,12 +117,37 @@ padding: 5px 0; font-size: 18px; line-height: 30px; + + .toggle-nav-collapse { + left: 0; + } + + .pin-nav-btn { + right: 0; + display: none; + + @media (min-width: $sidebar-breakpoint) { + display: block; + } + + .fa { + transition: transform .15s; + } + + &.is-active { + .fa { + transform: rotate(90deg); + } + } + } } .nav-header-btn { - padding: 10px 5px; + padding: 10px 16px; color: inherit; transition-duration: .3s; + position: absolute; + top: 0; &:hover, &:focus { @@ -131,30 +156,6 @@ } } -.pin-nav-btn { - display: none; - position: absolute; - left: 0; - bottom: 0; - height: 50px; - width: $sidebar_width; - line-height: 30px; - - @media (min-width: $sidebar-breakpoint) { - display: block; - } - - .fa { - transition: transform .15s; - } - - &.is-active { - .fa { - transform: rotate(90deg); - } - } -} - .page-sidebar-collapsed { padding-left: 0; diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 4337fab5d87..230ed28438a 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -86,6 +86,7 @@ $todo-alert-blue: #428bca; $btn-side-margin: 10px; $btn-sm-side-margin: 7px; $btn-xs-side-margin: 5px; +$box-shadow-gray: #bbb; /* * Color schema diff --git a/app/views/layouts/_collapse_button.html.haml b/app/views/layouts/_collapse_button.html.haml deleted file mode 100644 index 8c140a5943e..00000000000 --- a/app/views/layouts/_collapse_button.html.haml +++ /dev/null @@ -1,3 +0,0 @@ -= link_to '#', class: 'nav-header-btn text-center toggle-nav-collapse', title: "Open/Close" do - %span.sr-only Toggle navigation - = icon('bars') diff --git a/app/views/layouts/_page.html.haml b/app/views/layouts/_page.html.haml index 8596bbfdef6..90c1fa4c87c 100644 --- a/app/views/layouts/_page.html.haml +++ b/app/views/layouts/_page.html.haml @@ -1,6 +1,13 @@ .page-with-sidebar{ class: "#{page_sidebar_class} #{page_gutter_class}" } .sidebar-wrapper.nicescroll{ class: nav_sidebar_class } - = render partial: 'layouts/collapse_button' + .sidebar-action-buttons + = link_to '#', class: 'nav-header-btn toggle-nav-collapse', title: "Open/Close" do + %span.sr-only Toggle navigation + = icon('bars') + = link_to '#', class: "nav-header-btn pin-nav-btn has-tooltip #{'is-active' if pinned_nav?} js-nav-pin", title: pinned_nav? ? "Unpin navigation" : "Pin Navigation", data: {placement: 'right', container: 'body'} do + %span.sr-only Toggle navigation pinning + = icon('thumb-tack') + - if defined?(sidebar) && sidebar = render "layouts/nav/#{sidebar}" - elsif current_user @@ -8,9 +15,6 @@ - else = render 'layouts/nav/explore' - = link_to '#', class: "nav-header-btn text-center pin-nav-btn has-tooltip #{'is-active' if pinned_nav?} js-nav-pin", title: pinned_nav? ? "Unpin navigation" : "Pin Navigation", data: {placement: 'right', container: 'body'} do - %span.sr-only Toggle navigation pinning - = icon('thumb-tack') - if defined?(nav) && nav .layout-nav .container-fluid -- cgit v1.2.1 From adc6ec4a9c78029348ad65a18718f2a245714932 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 11 Jul 2016 17:12:31 -0500 Subject: Avoid `describe`-ing symbols in specs --- spec/finders/notes_finder_spec.rb | 2 +- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 4 ++-- spec/lib/gitlab/build_data_builder_spec.rb | 2 +- spec/lib/gitlab/diff/file_spec.rb | 4 ++-- spec/lib/gitlab/diff/parser_spec.rb | 2 +- spec/lib/gitlab/ldap/access_spec.rb | 2 +- spec/lib/gitlab/ldap/user_spec.rb | 2 +- spec/models/ci/pipeline_spec.rb | 10 ++++----- spec/models/ci/variable_spec.rb | 2 +- spec/models/commit_status_spec.rb | 14 ++++++------- spec/models/concerns/mentionable_spec.rb | 2 +- spec/models/forked_project_link_spec.rb | 2 +- spec/models/generic_commit_status_spec.rb | 10 ++++----- spec/models/global_milestone_spec.rb | 6 +++--- spec/models/group_spec.rb | 10 ++++----- spec/models/members/project_member_spec.rb | 4 ++-- spec/models/milestone_spec.rb | 10 ++++----- spec/models/namespace_spec.rb | 8 ++++---- spec/models/project_security_spec.rb | 2 +- .../project_services/buildkite_service_spec.rb | 6 +++--- spec/models/project_spec.rb | 24 +++++++++++----------- spec/models/repository_spec.rb | 20 +++++++++--------- spec/models/service_spec.rb | 6 +++--- spec/models/user_spec.rb | 12 +++++------ .../ci/create_trigger_request_service_spec.rb | 2 +- spec/services/ci/image_for_build_service_spec.rb | 2 +- spec/services/ci/register_build_service_spec.rb | 2 +- spec/services/create_commit_builds_service_spec.rb | 2 +- spec/services/event_create_service_spec.rb | 20 +++++++++--------- spec/services/issues/close_service_spec.rb | 2 +- spec/services/merge_requests/close_service_spec.rb | 2 +- .../services/merge_requests/create_service_spec.rb | 2 +- spec/services/merge_requests/merge_service_spec.rb | 2 +- .../merge_requests/refresh_service_spec.rb | 2 +- .../services/merge_requests/reopen_service_spec.rb | 2 +- spec/services/milestones/close_service_spec.rb | 2 +- spec/services/milestones/create_service_spec.rb | 2 +- spec/services/notes/create_service_spec.rb | 2 +- spec/services/notes/post_process_service_spec.rb | 2 +- spec/services/notification_service_spec.rb | 4 ++-- spec/services/test_hook_service_spec.rb | 2 +- 41 files changed, 110 insertions(+), 110 deletions(-) diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 1bd354815e4..8db897b1646 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -11,7 +11,7 @@ describe NotesFinder do project.team << [user, :master] end - describe :execute do + describe '#execute' do let(:params) { { target_id: commit.id, target_type: 'commit', last_fetched_at: 1.hour.ago.to_i } } before do diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index bad439bc489..bcbf409c8b0 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -31,7 +31,7 @@ module Ci }) end - describe :only do + describe 'only' do it "does not return builds if only has another branch" do config = YAML.dump({ before_script: ["pwd"], @@ -187,7 +187,7 @@ module Ci end end - describe :except do + describe 'except' do it "returns builds if except has another branch" do config = YAML.dump({ before_script: ["pwd"], diff --git a/spec/lib/gitlab/build_data_builder_spec.rb b/spec/lib/gitlab/build_data_builder_spec.rb index 38be9448794..23ae5cfacc4 100644 --- a/spec/lib/gitlab/build_data_builder_spec.rb +++ b/spec/lib/gitlab/build_data_builder_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'Gitlab::BuildDataBuilder' do let(:build) { create(:ci_build) } - describe :build do + describe '.build' do let(:data) do Gitlab::BuildDataBuilder.build(build) end diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 1cb513d5229..0460dcf4658 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -8,14 +8,14 @@ describe Gitlab::Diff::File, lib: true do let(:diff) { commit.diffs.first } let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: project.repository) } - describe :diff_lines do + describe '#diff_lines' do let(:diff_lines) { diff_file.diff_lines } it { expect(diff_lines.size).to eq(30) } it { expect(diff_lines.first).to be_kind_of(Gitlab::Diff::Line) } end - describe :mode_changed? do + describe '#mode_changed?' do it { expect(diff_file.mode_changed?).to be_falsey } end diff --git a/spec/lib/gitlab/diff/parser_spec.rb b/spec/lib/gitlab/diff/parser_spec.rb index cdff063a9ed..c3359627652 100644 --- a/spec/lib/gitlab/diff/parser_spec.rb +++ b/spec/lib/gitlab/diff/parser_spec.rb @@ -8,7 +8,7 @@ describe Gitlab::Diff::Parser, lib: true do let(:diff) { commit.diffs.first } let(:parser) { Gitlab::Diff::Parser.new } - describe :parse do + describe '#parse' do let(:diff) do < Date: Tue, 12 Jul 2016 10:43:43 -0500 Subject: Change sidebar drop shadow to rgba --- app/assets/stylesheets/framework/sidebar.scss | 2 +- app/assets/stylesheets/framework/variables.scss | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 99833ef842a..cec52678495 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -17,7 +17,7 @@ height: 100%; overflow: hidden; transition: width $sidebar-transition-duration; - @include box-shadow(2px 0 16px 0 $box-shadow-gray); + @include box-shadow(2px 0 16px 0 $black-transparent); } } diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 230ed28438a..4337fab5d87 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -86,7 +86,6 @@ $todo-alert-blue: #428bca; $btn-side-margin: 10px; $btn-sm-side-margin: 7px; $btn-xs-side-margin: 5px; -$box-shadow-gray: #bbb; /* * Color schema -- cgit v1.2.1 From e981d6cd0d6b8ae7d2911a57a65751deaa9ec555 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 12 Jul 2016 11:25:39 -0500 Subject: Extended regexes ignore whitespace, so use \s --- lib/gitlab/diff/inline_diff.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/diff/inline_diff.rb b/lib/gitlab/diff/inline_diff.rb index 72d9abeefcc..4a3eb92b9fc 100644 --- a/lib/gitlab/diff/inline_diff.rb +++ b/lib/gitlab/diff/inline_diff.rb @@ -4,7 +4,7 @@ module Gitlab # Regex to find a run of deleted lines followed by the same number of added lines REGEX = %r{ # Runs start at the beginning of the string (the first line) or after a space (for an unchanged line) - (?:\A| ) + (?:\A|\s) # This matches a number of `-`s followed by the same number of `+`s through recursion (? @@ -14,7 +14,7 @@ module Gitlab ) # Runs end at the end of the string (the last line) or before a space (for an unchanged line) - (?= |\z) + (?=\s|\z) }x.freeze attr_accessor :old_line, :new_line, :offset -- cgit v1.2.1 From c39356998b1850f3dc26fe0b987cb419c1d1afb4 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Fri, 8 Jul 2016 00:42:16 +0300 Subject: Optimistic locking for Issue and Merge Requests --- CHANGELOG | 1 + app/controllers/projects/issues_controller.rb | 6 +++++- .../projects/merge_requests_controller.rb | 5 ++++- app/models/concerns/issuable.rb | 6 ++++++ app/views/shared/issuable/_form.html.haml | 9 ++++++++ db/migrate/20160707104333_add_lock_to_issuables.rb | 17 +++++++++++++++ db/schema.rb | 24 ++++++++++++---------- features/project/merge_requests.feature | 2 +- spec/features/issues_spec.rb | 11 ++++++++++ spec/features/merge_requests/edit_mr_spec.rb | 11 ++++++++++ 10 files changed, 78 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20160707104333_add_lock_to_issuables.rb diff --git a/CHANGELOG b/CHANGELOG index a1d44d02bc5..4dd96f0287a 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -81,6 +81,7 @@ v 8.10.0 (unreleased) - Remove duplicate `description` field in `MergeRequest` entities (Ben Boeckel) - Style of import project buttons were fixed in the new project page. !5183 (rdemirbay) - Fix GitHub client requests when rate limit is disabled + - Optimistic locking for Issues and Merge Requests (Title and description overriding prevention) v 8.9.6 (unreleased) - Fix importing of events under notes for GitLab projects diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index b6e80762e3c..f7ada5cfee4 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -119,6 +119,10 @@ class Projects::IssuesController < Projects::ApplicationController render json: @issue.to_json(include: { milestone: {}, assignee: { methods: :avatar_url }, labels: { methods: :text_color } }) end end + + rescue ActiveRecord::StaleObjectError + @conflict = true + render :edit end def referenced_merge_requests @@ -216,7 +220,7 @@ class Projects::IssuesController < Projects::ApplicationController def issue_params params.require(:issue).permit( :title, :assignee_id, :position, :description, :confidential, - :milestone_id, :due_date, :state_event, :task_num, label_ids: [] + :milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: [] ) end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index df659bb8c3b..2deb7959700 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -196,6 +196,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController else render "edit" end + rescue ActiveRecord::StaleObjectError + @conflict = true + render :edit end def remove_wip @@ -424,7 +427,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController :title, :assignee_id, :source_project_id, :source_branch, :target_project_id, :target_branch, :milestone_id, :state_event, :description, :task_num, :force_remove_source_branch, - label_ids: [] + :lock_version, label_ids: [] ) end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index acb6f5a2998..fb49bd7dd64 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -87,6 +87,12 @@ module Issuable User.find(assignee_id_was).update_cache_counts if assignee_id_was assignee.update_cache_counts if assignee end + + # We want to use optimistic lock for cases when only title or description are involved + # http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html + def locking_enabled? + title_changed? || description_changed? + end end module ClassMethods diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index c30bdb0ae91..98bbb12eaec 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -1,5 +1,12 @@ = form_errors(issuable) +- if @conflict + .alert.alert-danger + Someone edited the #{issuable.class.model_name.human.downcase} the same time you did. + Please check out + = link_to "the #{issuable.class.model_name.human.downcase}", polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), target: "_blank" + and make sure your changes will not unintentionally remove theirs + .form-group = f.label :title, class: 'control-label' .col-sm-10 @@ -149,3 +156,5 @@ = link_to 'Delete', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), data: { confirm: "#{issuable.class.name.titleize} will be removed! Are you sure?" }, method: :delete, class: 'btn btn-danger btn-grouped' = link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), class: 'btn btn-grouped btn-cancel' + += f.hidden_field :lock_version diff --git a/db/migrate/20160707104333_add_lock_to_issuables.rb b/db/migrate/20160707104333_add_lock_to_issuables.rb new file mode 100644 index 00000000000..cb516672800 --- /dev/null +++ b/db/migrate/20160707104333_add_lock_to_issuables.rb @@ -0,0 +1,17 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddLockToIssuables < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + def up + add_column_with_default :issues, :lock_version, :integer, default: 0 + add_column_with_default :merge_requests, :lock_version, :integer, default: 0 + end + + def down + remove_column :issues, :lock_version + remove_column :merge_requests, :lock_version + end +end diff --git a/db/schema.rb b/db/schema.rb index a5eea3a697c..9d31947d80f 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: 20160705163108) do +ActiveRecord::Schema.define(version: 20160707104333) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -70,11 +70,11 @@ ActiveRecord::Schema.define(version: 20160705163108) do t.string "recaptcha_site_key" t.string "recaptcha_private_key" t.integer "metrics_port", default: 8089 - t.boolean "akismet_enabled", default: false - t.string "akismet_api_key" t.integer "metrics_sample_interval", default: 15 t.boolean "sentry_enabled", default: false t.string "sentry_dsn" + t.boolean "akismet_enabled", default: false + t.string "akismet_api_key" t.boolean "email_author_in_body", default: false t.integer "default_group_visibility" t.boolean "repository_checks_enabled", default: false @@ -84,10 +84,10 @@ ActiveRecord::Schema.define(version: 20160705163108) do t.string "health_check_access_token" t.boolean "send_user_confirmation_email", default: false t.integer "container_registry_token_expire_delay", default: 5 - t.boolean "user_default_external", default: false, null: false t.text "after_sign_up_text" t.string "repository_storage", default: "default" t.string "enabled_git_access_protocol" + t.boolean "user_default_external", default: false, null: false end create_table "audit_events", force: :cascade do |t| @@ -165,8 +165,8 @@ ActiveRecord::Schema.define(version: 20160705163108) do t.text "artifacts_metadata" t.integer "erased_by_id" t.datetime "erased_at" - t.string "environment" t.datetime "artifacts_expire_at" + t.string "environment" t.integer "artifacts_size" end @@ -481,10 +481,11 @@ ActiveRecord::Schema.define(version: 20160705163108) do t.string "state" t.integer "iid" t.integer "updated_by_id" + t.integer "moved_to_id" t.boolean "confidential", default: false t.datetime "deleted_at" t.date "due_date" - t.integer "moved_to_id" + t.integer "lock_version", default: 0, null: false end add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree @@ -624,6 +625,7 @@ ActiveRecord::Schema.define(version: 20160705163108) do t.integer "merge_user_id" t.string "merge_commit_sha" t.datetime "deleted_at" + t.integer "lock_version", default: 0, null: false end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree @@ -773,10 +775,10 @@ ActiveRecord::Schema.define(version: 20160705163108) do t.integer "user_id", null: false t.string "token", null: false t.string "name", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false t.boolean "revoked", default: false t.datetime "expires_at" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false end add_index "personal_access_tokens", ["token"], name: "index_personal_access_tokens_on_token", unique: true, using: :btree @@ -896,9 +898,9 @@ ActiveRecord::Schema.define(version: 20160705163108) do t.string "type" t.string "title" t.integer "project_id" - t.datetime "created_at" - t.datetime "updated_at" - t.boolean "active", default: false, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.boolean "active", null: false t.text "properties" t.boolean "template", default: false t.boolean "push_events", default: true diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 21768c15c17..8176ec5ab45 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -89,7 +89,7 @@ Feature: Project Merge Requests Then The list should be sorted by "Oldest updated" @javascript - Scenario: Visiting Merge Requests from a differente Project after sorting + Scenario: Visiting Merge Requests from a different Project after sorting Given I visit project "Shop" merge requests page And I sort the list by "Oldest updated" And I visit dashboard merge requests page diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb index d51c9abea19..cfe6349a1a1 100644 --- a/spec/features/issues_spec.rb +++ b/spec/features/issues_spec.rb @@ -121,6 +121,17 @@ describe 'Issues', feature: true do expect(page).to have_content date.to_s(:medium) end end + + it 'warns about version conflict' do + issue.update(title: "New title") + + fill_in 'issue_title', with: 'bug 345' + fill_in 'issue_description', with: 'bug description' + + click_button 'Save changes' + + expect(page).to have_content 'Someone edited the issue the same time you did' + end end end diff --git a/spec/features/merge_requests/edit_mr_spec.rb b/spec/features/merge_requests/edit_mr_spec.rb index 9e007ab7635..8ad884492d1 100644 --- a/spec/features/merge_requests/edit_mr_spec.rb +++ b/spec/features/merge_requests/edit_mr_spec.rb @@ -17,5 +17,16 @@ feature 'Edit Merge Request', feature: true do it 'form should have class js-quick-submit' do expect(page).to have_selector('.js-quick-submit') end + + it 'warns about version conflict' do + merge_request.update(title: "New title") + + fill_in 'merge_request_title', with: 'bug 345' + fill_in 'merge_request_description', with: 'bug description' + + click_button 'Save changes' + + expect(page).to have_content 'Someone edited the merge request the same time you did' + end end end -- cgit v1.2.1 From 244134f9c33dea0003dc2403dceace4b94a87d2e Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Mon, 11 Jul 2016 08:10:04 +0200 Subject: Cache todos pending/done dashboard query counts --- CHANGELOG | 1 + app/controllers/dashboard/todos_controller.rb | 13 ++-- app/controllers/projects/todos_controller.rb | 2 +- app/finders/todos_finder.rb | 2 +- app/helpers/todos_helper.rb | 4 +- spec/controllers/projects/todo_controller_spec.rb | 78 ++++++++++++++--------- 6 files changed, 62 insertions(+), 38 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index ee3ee4c37d6..3758baa0a0a 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -49,6 +49,7 @@ v 8.10.0 (unreleased) - Collapse large diffs by default (!4990) - Check for conflicts with existing Project's wiki path when creating a new project. - Show last push widget in upstream after push to fork + - Cache todos pending/done dashboard query counts. - Don't instantiate a git tree on Projects show default view - Bump Rinku to 2.0.0 - Remove unused front-end variable -> default_issues_tracker diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index 3a2db3e6eeb..19a76a5b5d8 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -1,6 +1,4 @@ class Dashboard::TodosController < Dashboard::ApplicationController - include TodosHelper - before_action :find_todos, only: [:index, :destroy_all] def index @@ -13,7 +11,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController respond_to do |format| format.html { redirect_to dashboard_todos_path, notice: 'Todo was successfully marked as done.' } format.js { head :ok } - format.json { render json: { count: todos_pending_count, done_count: todos_done_count } } + format.json { render json: todos_counts } end end @@ -23,7 +21,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController respond_to do |format| format.html { redirect_to dashboard_todos_path, notice: 'All todos were marked as done.' } format.js { head :ok } - format.json { render json: { count: todos_pending_count, done_count: todos_done_count } } + format.json { render json: todos_counts } end end @@ -36,4 +34,11 @@ class Dashboard::TodosController < Dashboard::ApplicationController def find_todos @todos ||= TodosFinder.new(current_user, params).execute end + + def todos_counts + { + count: TodosFinder.new(current_user, state: :pending).execute.count, + done_count: TodosFinder.new(current_user, state: :done).execute.count + } + end end diff --git a/app/controllers/projects/todos_controller.rb b/app/controllers/projects/todos_controller.rb index 23868d986e9..5685d0f4e7c 100644 --- a/app/controllers/projects/todos_controller.rb +++ b/app/controllers/projects/todos_controller.rb @@ -5,7 +5,7 @@ class Projects::TodosController < Projects::ApplicationController todo = TodoService.new.mark_todo(issuable, current_user) render json: { - count: current_user.todos_pending_count, + count: TodosFinder.new(current_user, state: :pending).execute.count, delete_path: dashboard_todo_path(todo) } end diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 7806d9e4cc5..1a8f490355b 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -8,7 +8,7 @@ # action_id: integer # author_id: integer # project_id; integer -# state: 'pending' or 'done' +# state: 'pending' (default) or 'done' # type: 'Issue' or 'MergeRequest' # diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index a832a6c8df7..0925760e69c 100644 --- a/app/helpers/todos_helper.rb +++ b/app/helpers/todos_helper.rb @@ -1,10 +1,10 @@ module TodosHelper def todos_pending_count - TodosFinder.new(current_user, state: :pending).execute.count + @todos_pending_count ||= TodosFinder.new(current_user, state: :pending).execute.count end def todos_done_count - TodosFinder.new(current_user, state: :done).execute.count + @todos_done_count ||= TodosFinder.new(current_user, state: :done).execute.count end def todo_action_name(todo) diff --git a/spec/controllers/projects/todo_controller_spec.rb b/spec/controllers/projects/todo_controller_spec.rb index 5a8bba28594..936320a3709 100644 --- a/spec/controllers/projects/todo_controller_spec.rb +++ b/spec/controllers/projects/todo_controller_spec.rb @@ -1,6 +1,8 @@ require('spec_helper') describe Projects::TodosController do + include ApiHelpers + let(:user) { create(:user) } let(:project) { create(:project) } let(:issue) { create(:issue, project: project) } @@ -8,43 +10,51 @@ describe Projects::TodosController do context 'Issues' do describe 'POST create' do + def go + post :create, + namespace_id: project.namespace.path, + project_id: project.path, + issuable_id: issue.id, + issuable_type: 'issue', + format: 'html' + end + context 'when authorized' do before do sign_in(user) project.team << [user, :developer] end - it 'should create todo for issue' do + it 'creates todo for issue' do expect do - post(:create, namespace_id: project.namespace.path, - project_id: project.path, - issuable_id: issue.id, - issuable_type: 'issue') + go end.to change { user.todos.count }.by(1) expect(response).to have_http_status(200) end + + it 'returns todo path and pending count' do + go + + expect(response).to have_http_status(200) + expect(json_response['count']).to eq 1 + expect(json_response['delete_path']).to match(/\/dashboard\/todos\/\d{1}/) + end end context 'when not authorized' do - it 'should not create todo for issue that user has no access to' do + it 'does not create todo for issue that user has no access to' do sign_in(user) expect do - post(:create, namespace_id: project.namespace.path, - project_id: project.path, - issuable_id: issue.id, - issuable_type: 'issue') + go end.to change { user.todos.count }.by(0) expect(response).to have_http_status(404) end - it 'should not create todo for issue when user not logged in' do + it 'does not create todo for issue when user not logged in' do expect do - post(:create, namespace_id: project.namespace.path, - project_id: project.path, - issuable_id: issue.id, - issuable_type: 'issue') + go end.to change { user.todos.count }.by(0) expect(response).to have_http_status(302) @@ -55,43 +65,51 @@ describe Projects::TodosController do context 'Merge Requests' do describe 'POST create' do + def go + post :create, + namespace_id: project.namespace.path, + project_id: project.path, + issuable_id: merge_request.id, + issuable_type: 'merge_request', + format: 'html' + end + context 'when authorized' do before do sign_in(user) project.team << [user, :developer] end - it 'should create todo for merge request' do + it 'creates todo for merge request' do expect do - post(:create, namespace_id: project.namespace.path, - project_id: project.path, - issuable_id: merge_request.id, - issuable_type: 'merge_request') + go end.to change { user.todos.count }.by(1) expect(response).to have_http_status(200) end + + it 'returns todo path and pending count' do + go + + expect(response).to have_http_status(200) + expect(json_response['count']).to eq 1 + expect(json_response['delete_path']).to match(/\/dashboard\/todos\/\d{1}/) + end end context 'when not authorized' do - it 'should not create todo for merge request user has no access to' do + it 'does not create todo for merge request user has no access to' do sign_in(user) expect do - post(:create, namespace_id: project.namespace.path, - project_id: project.path, - issuable_id: merge_request.id, - issuable_type: 'merge_request') + go end.to change { user.todos.count }.by(0) expect(response).to have_http_status(404) end - it 'should not create todo for merge request user has no access to' do + it 'does not create todo for merge request user has no access to' do expect do - post(:create, namespace_id: project.namespace.path, - project_id: project.path, - issuable_id: merge_request.id, - issuable_type: 'merge_request') + go end.to change { user.todos.count }.by(0) expect(response).to have_http_status(302) -- cgit v1.2.1 From 47b5b441395921e9f8e9982bb3f560e5db5a67bc Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 12 Jul 2016 17:22:10 +0200 Subject: Defend against 'Host' header injection Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/17877 . This change adds 'defense in depth' against 'Host' HTTP header injection. It affects normal users in the following way. Suppose your GitLab server has IP address 1.2.3.4 and hostname gitlab.example.com. Currently, if you enter 1.2.3.4 in your browser, you get redirected to 1.2.3.4/users/sign_in. After this change, you get redirected from 1.2.3.4 to gitlab.example.com/users/sign_in. This is because the address you typed in the address bar of your browser ('1.2.3.4'), which gets stored in the 'Host' header, is now being overwritten to 'gitlab.example.com' in NGINX. In this change we also make NGINX clear the 'X-Forwarded-Host' header because Ruby on Rails also uses that header the same wayas the 'Host' header. We think that for most GitLab servers this is the right behavior, and if not then administrators can change this behavior themselves at the NGINX level. --- CHANGELOG | 1 + lib/support/nginx/gitlab | 7 ++++++- lib/support/nginx/gitlab-ssl | 7 ++++++- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index ee3ee4c37d6..852a123f0cd 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -78,6 +78,7 @@ v 8.10.0 (unreleased) - Add reminder to not paste private SSH keys !4399 (Ingo Blechschmidt) - Remove duplicate `description` field in `MergeRequest` entities (Ben Boeckel) - Style of import project buttons were fixed in the new project page. !5183 (rdemirbay) + - Overwrite Host and X-Forwarded-Host headers in NGINX !5213 v 8.9.6 (unreleased) - Fix importing of events under notes for GitLab projects diff --git a/lib/support/nginx/gitlab b/lib/support/nginx/gitlab index d521de28e8a..4a4892a2e07 100644 --- a/lib/support/nginx/gitlab +++ b/lib/support/nginx/gitlab @@ -49,7 +49,12 @@ server { proxy_http_version 1.1; - proxy_set_header Host $http_host; + ## By overwriting Host and clearing X-Forwarded-Host we ensure that + ## internal HTTP redirects generated by GitLab always send users to + ## YOUR_SERVER_FQDN. + proxy_set_header Host YOUR_SERVER_FQDN; + proxy_set_header X-Forwarded-Host ""; + proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; diff --git a/lib/support/nginx/gitlab-ssl b/lib/support/nginx/gitlab-ssl index bf014b56cf6..0b93d7f292f 100644 --- a/lib/support/nginx/gitlab-ssl +++ b/lib/support/nginx/gitlab-ssl @@ -93,7 +93,12 @@ server { proxy_http_version 1.1; - proxy_set_header Host $http_host; + ## By overwriting Host and clearing X-Forwarded-Host we ensure that + ## internal HTTP redirects generated by GitLab always send users to + ## YOUR_SERVER_FQDN. + proxy_set_header Host YOUR_SERVER_FQDN; + proxy_set_header X-Forwarded-Host ""; + proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-Ssl on; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; -- cgit v1.2.1 From d957d5f1aa45d3a8474678e5439ccdc09a545482 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 11 Jul 2016 11:02:11 +0100 Subject: Add approval required todos --- app/finders/todos_finder.rb | 2 +- app/helpers/todos_helper.rb | 1 + app/models/todo.rb | 12 +++++++----- doc/api/todos.md | 2 +- spec/factories/todos.rb | 4 ++++ 5 files changed, 14 insertions(+), 7 deletions(-) diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 7806d9e4cc5..681b403b817 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -37,7 +37,7 @@ class TodosFinder private def action_id? - action_id.present? && [Todo::ASSIGNED, Todo::MENTIONED, Todo::BUILD_FAILED, Todo::MARKED].include?(action_id.to_i) + action_id.present? && Todo::ACTION_NAMES.has_key?(action_id.to_i) end def action_id diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index a832a6c8df7..e43c209f90c 100644 --- a/app/helpers/todos_helper.rb +++ b/app/helpers/todos_helper.rb @@ -13,6 +13,7 @@ module TodosHelper when Todo::MENTIONED then 'mentioned you on' when Todo::BUILD_FAILED then 'The build failed for your' when Todo::MARKED then 'added a todo for' + when Todo::APPROVAL_REQUIRED then 'set you as an approver for' end end diff --git a/app/models/todo.rb b/app/models/todo.rb index ac3fdbc7f3b..8d7a5965aa1 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -1,14 +1,16 @@ class Todo < ActiveRecord::Base - ASSIGNED = 1 - MENTIONED = 2 - BUILD_FAILED = 3 - MARKED = 4 + ASSIGNED = 1 + MENTIONED = 2 + BUILD_FAILED = 3 + MARKED = 4 + APPROVAL_REQUIRED = 5 # This is an EE-only feature ACTION_NAMES = { ASSIGNED => :assigned, MENTIONED => :mentioned, BUILD_FAILED => :build_failed, - MARKED => :marked + MARKED => :marked, + APPROVAL_REQUIRED => :approval_required } belongs_to :author, class_name: "User" diff --git a/doc/api/todos.md b/doc/api/todos.md index 29e73664410..23f6e35f2a4 100644 --- a/doc/api/todos.md +++ b/doc/api/todos.md @@ -15,7 +15,7 @@ Parameters: | Attribute | Type | Required | Description | | --------- | ---- | -------- | ----------- | -| `action` | string | no | The action to be filtered. Can be `assigned`, `mentioned`, `build_failed`, or `marked`. | +| `action` | string | no | The action to be filtered. Can be `assigned`, `mentioned`, `build_failed`, `marked`, or `approval_required`. | | `author_id` | integer | no | The ID of an author | | `project_id` | integer | no | The ID of a project | | `state` | string | no | The state of the todo. Can be either `pending` or `done` | diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb index 7fc20cd5555..866e663f026 100644 --- a/spec/factories/todos.rb +++ b/spec/factories/todos.rb @@ -23,6 +23,10 @@ FactoryGirl.define do action { Todo::BUILD_FAILED } end + trait :approval_required do + action { Todo::APPROVAL_REQUIRED } + end + trait :done do state :done end -- cgit v1.2.1 From 144d22f7f87c270cb5192366c6e76dfad2a2c389 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 12 Jul 2016 18:57:08 +0100 Subject: Delete award emoji when deleting a user --- CHANGELOG | 3 ++- app/models/user.rb | 2 +- ...160712171823_remove_award_emojis_with_no_user.rb | 21 +++++++++++++++++++++ db/schema.rb | 2 +- 4 files changed, 25 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20160712171823_remove_award_emojis_with_no_user.rb diff --git a/CHANGELOG b/CHANGELOG index a50edf1a797..1a4d792ef84 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -8,6 +8,7 @@ v 8.10.0 (unreleased) - Refactor repository paths handling to allow multiple git mount points - Optimize system note visibility checking by memoizing the visible reference count !5070 - Add Application Setting to configure default Repository Path for new projects + - Delete award emoji when deleting a user - Remove pinTo from Flash and make inline flash messages look nicer !4854 (winniehell) - Wrap code blocks on Activies and Todos page. !4783 (winniehell) - Align flash messages with left side of page content !4959 (winniehell) @@ -73,7 +74,7 @@ v 8.10.0 (unreleased) - Allow '?', or '&' for label names - Fix importer for GitHub Pull Requests when a branch was reused across Pull Requests - Add date when user joined the team on the member page - - Fix 404 redirect after validation fails importing a GitLab project + - Fix 404 redirect after validation fails importing a GitLab project - Fix 404 redirect after validation fails importing a GitLab project - Added setting to set new users by default as external !4545 (Dravere) - Add min value for project limit field on user's form !3622 (jastkand) diff --git a/app/models/user.rb b/app/models/user.rb index 79c670cb35a..7a72c202150 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -87,7 +87,7 @@ class User < ActiveRecord::Base has_many :builds, dependent: :nullify, class_name: 'Ci::Build' has_many :todos, dependent: :destroy has_many :notification_settings, dependent: :destroy - has_many :award_emoji, as: :awardable, dependent: :destroy + has_many :award_emoji, dependent: :destroy # # Validations diff --git a/db/migrate/20160712171823_remove_award_emojis_with_no_user.rb b/db/migrate/20160712171823_remove_award_emojis_with_no_user.rb new file mode 100644 index 00000000000..668c22bb51c --- /dev/null +++ b/db/migrate/20160712171823_remove_award_emojis_with_no_user.rb @@ -0,0 +1,21 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RemoveAwardEmojisWithNoUser < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # 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 + AwardEmoji.joins('LEFT JOIN users ON users.id = user_id').where('users.id IS NULL').destroy_all + end +end diff --git a/db/schema.rb b/db/schema.rb index 9d31947d80f..f24e47b85b2 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: 20160707104333) do +ActiveRecord::Schema.define(version: 20160712171823) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" -- cgit v1.2.1 From 47199c35a5555483309cdbde39c3da51e5a75d69 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Thu, 7 Jul 2016 08:44:00 -0500 Subject: Add clock and calendar icons for duration and finished at; add finished at section in pipelines --- app/views/projects/ci/builds/_build.html.haml | 2 ++ app/views/projects/ci/pipelines/_pipeline.html.haml | 5 +++++ .../generic_commit_statuses/_generic_commit_status.html.haml | 2 ++ 3 files changed, 9 insertions(+) diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index 5bd6e3f0ebc..00004163047 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -57,10 +57,12 @@ %td.duration - if build.duration + = icon("clock-o") #{duration_in_words(build.finished_at, build.started_at)} %td.timestamp - if build.finished_at + = icon("calendar") %span #{time_ago_with_tooltip(build.finished_at)} - if defined?(coverage) && coverage diff --git a/app/views/projects/ci/pipelines/_pipeline.html.haml b/app/views/projects/ci/pipelines/_pipeline.html.haml index af8dd5cd02c..f2c19797206 100644 --- a/app/views/projects/ci/pipelines/_pipeline.html.haml +++ b/app/views/projects/ci/pipelines/_pipeline.html.haml @@ -45,7 +45,12 @@ %td - if pipeline.started_at && pipeline.finished_at %p.duration + = icon("clock-o") = duration_in_numbers(pipeline.finished_at, pipeline.started_at) + - if pipeline.finished_at + %p.duration + = icon("calendar") + #{time_ago_with_tooltip(pipeline.finished_at)} %td .controls.hidden-xs.pull-right diff --git a/app/views/projects/generic_commit_statuses/_generic_commit_status.html.haml b/app/views/projects/generic_commit_statuses/_generic_commit_status.html.haml index 5bc5c71283e..542827b2f15 100644 --- a/app/views/projects/generic_commit_statuses/_generic_commit_status.html.haml +++ b/app/views/projects/generic_commit_statuses/_generic_commit_status.html.haml @@ -50,10 +50,12 @@ %td.duration - if generic_commit_status.duration + = icon("clock-o") #{duration_in_words(generic_commit_status.finished_at, generic_commit_status.started_at)} %td.timestamp - if generic_commit_status.finished_at + = icon("calendar") %span #{time_ago_with_tooltip(generic_commit_status.finished_at)} - if defined?(coverage) && coverage -- cgit v1.2.1 From 446eecb85275352dcd4c3e2c6054461da2a51ca7 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Thu, 7 Jul 2016 13:40:40 -0500 Subject: Move pipeline ID to commit column; add status; branch style updates --- app/assets/stylesheets/pages/builds.scss | 32 +++++++++++++++++++++- .../projects/ci/pipelines/_pipeline.html.haml | 9 +++--- app/views/projects/pipelines/index.html.haml | 4 +-- 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/app/assets/stylesheets/pages/builds.scss b/app/assets/stylesheets/pages/builds.scss index e8f1935d239..76024933650 100644 --- a/app/assets/stylesheets/pages/builds.scss +++ b/app/assets/stylesheets/pages/builds.scss @@ -83,7 +83,37 @@ } } -table.builds { + +.table.builds { + + tr { + th { + padding: 18px 10px; + border: none; + } + } + + tbody { + border-top-width: 1px; + } + + .branch-commit { + + .branch-name { + max-width: 180px; + overflow: hidden; + display: inline-block; + white-space: nowrap; + vertical-align: top; + text-overflow: ellipsis; + margin-left: 10px; + } + + .commit-id { + color: $gl-link-color; + } + } + .build-link { a { color: $gl-dark-link-color; diff --git a/app/views/projects/ci/pipelines/_pipeline.html.haml b/app/views/projects/ci/pipelines/_pipeline.html.haml index f2c19797206..daeaf7f99eb 100644 --- a/app/views/projects/ci/pipelines/_pipeline.html.haml +++ b/app/views/projects/ci/pipelines/_pipeline.html.haml @@ -1,14 +1,15 @@ - status = pipeline.status %tr.commit %td.commit-link - = link_to namespace_project_pipeline_path(@project.namespace, @project, pipeline.id), class: "ci-status ci-#{status}" do - = ci_icon_for_status(status) - %strong ##{pipeline.id} + = link_to namespace_project_pipeline_path(@project.namespace, @project, pipeline.id) do + = ci_status_with_icon(status) + %td %div.branch-commit + %span ##{pipeline.id} - if pipeline.ref - = link_to pipeline.ref, namespace_project_commits_path(@project.namespace, @project, pipeline.ref), class: "monospace" + = link_to pipeline.ref, namespace_project_commits_path(@project.namespace, @project, pipeline.ref), class: "monospace branch-name" · = link_to pipeline.short_sha, namespace_project_commit_path(@project.namespace, @project, pipeline.sha), class: "commit-id monospace"   diff --git a/app/views/projects/pipelines/index.html.haml b/app/views/projects/pipelines/index.html.haml index 7c225e2b282..e1c82a8179d 100644 --- a/app/views/projects/pipelines/index.html.haml +++ b/app/views/projects/pipelines/index.html.haml @@ -45,13 +45,13 @@ .table-holder %table.table.builds %tbody - %th ID + %th Status %th Commit - stages.each do |stage| %th.stage %span.has-tooltip{ title: "#{stage.titleize}" } = stage.titleize - %th Duration + %%th %th = render @pipelines, commit_sha: true, stage: true, allow_retry: true, stages: stages -- cgit v1.2.1 From 1f67cc4a559e72bedf06f169343c504fcfdd3d49 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Fri, 8 Jul 2016 10:01:56 -0500 Subject: Set width on stage columns; min width on table with scroll on mobile; add avatar to commit column --- app/assets/stylesheets/framework/avatar.scss | 1 + app/assets/stylesheets/framework/variables.scss | 1 + app/assets/stylesheets/pages/builds.scss | 38 ---------- app/assets/stylesheets/pages/pipelines.scss | 87 +++++++++++++++++++++- .../projects/ci/pipelines/_pipeline.html.haml | 10 +-- app/views/projects/pipelines/index.html.haml | 2 +- 6 files changed, 93 insertions(+), 46 deletions(-) diff --git a/app/assets/stylesheets/framework/avatar.scss b/app/assets/stylesheets/framework/avatar.scss index bb8d71fbae8..8b6ddf8ba18 100644 --- a/app/assets/stylesheets/framework/avatar.scss +++ b/app/assets/stylesheets/framework/avatar.scss @@ -20,6 +20,7 @@ } &.s16 { width: 16px; height: 16px; margin-right: 6px; } + &.s20 { width: 20px; height: 20px; margin-right: 7px; } &.s24 { width: 24px; height: 24px; margin-right: 8px; } &.s26 { width: 26px; height: 26px; margin-right: 8px; } &.s32 { width: 32px; height: 32px; margin-right: 10px; } diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 4337fab5d87..09d3caa0e6a 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -17,6 +17,7 @@ $focus-border-color: #3aabf0; $table-border-color: #f0f0f0; $background-color: #fafafa; $dark-background-color: #f7f7f7; +$table-text-gray: #8f8f8f; /* * Text diff --git a/app/assets/stylesheets/pages/builds.scss b/app/assets/stylesheets/pages/builds.scss index 76024933650..99a2cd306cf 100644 --- a/app/assets/stylesheets/pages/builds.scss +++ b/app/assets/stylesheets/pages/builds.scss @@ -83,44 +83,6 @@ } } - -.table.builds { - - tr { - th { - padding: 18px 10px; - border: none; - } - } - - tbody { - border-top-width: 1px; - } - - .branch-commit { - - .branch-name { - max-width: 180px; - overflow: hidden; - display: inline-block; - white-space: nowrap; - vertical-align: top; - text-overflow: ellipsis; - margin-left: 10px; - } - - .commit-id { - color: $gl-link-color; - } - } - - .build-link { - a { - color: $gl-dark-link-color; - } - } -} - .build-trace { background: $ci-output-bg; color: $ci-text-color; diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index 6128868b670..fb5840a4f67 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -1,12 +1,13 @@ .pipelines { .stage { - max-width: 100px; + max-width: 70px; + width: 70px; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } - .duration, .finished_at { + .duration, .finished-at { margin: 4px 0; } @@ -22,3 +23,85 @@ margin: 4px; } } + +.content-list.pipelines { + width: 100%; + overflow: auto; +} + +.table.builds { + min-width: 1100px; + + tr { + th { + padding: 18px 10px; + border: none; + } + } + + tbody { + border-top-width: 1px; + } + + .branch-commit { + + .branch-name { + margin-left: 8px; + font-weight: bold; + max-width: 180px; + overflow: hidden; + display: inline-block; + white-space: nowrap; + vertical-align: top; + text-overflow: ellipsis; + } + + .fa { + margin: 0 6px; + } + + .commit-id { + color: $gl-link-color; + margin-right: 8px; + } + + .commit-title { + margin-top: 4px; + max-width: 320px; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; + } + + .avatar { + margin-left: 0; + } + } + + .duration, + .finished-at { + color: $table-text-gray; + + .fa { + margin-right: 5px; + } + } + + .pipeline-actions { + + .btn { + color: $table-text-gray; + } + + .btn-remove { + color: $white-light; + } + } + + .build-link { + + a { + color: $gl-dark-link-color; + } + } +} diff --git a/app/views/projects/ci/pipelines/_pipeline.html.haml b/app/views/projects/ci/pipelines/_pipeline.html.haml index daeaf7f99eb..b8712b5dc45 100644 --- a/app/views/projects/ci/pipelines/_pipeline.html.haml +++ b/app/views/projects/ci/pipelines/_pipeline.html.haml @@ -10,9 +10,8 @@ %span ##{pipeline.id} - if pipeline.ref = link_to pipeline.ref, namespace_project_commits_path(@project.namespace, @project, pipeline.ref), class: "monospace branch-name" - · + = icon("code-fork") = link_to pipeline.short_sha, namespace_project_commit_path(@project.namespace, @project, pipeline.sha), class: "commit-id monospace" -   - if pipeline.tag? %span.label.label-primary tag - elsif pipeline.latest? @@ -26,6 +25,7 @@ %p.commit-title - if commit = pipeline.commit + = commit_author_avatar(commit, size: 20) = link_to_gfm truncate(commit.title, length: 60), namespace_project_commit_path(@project.namespace, @project, commit.id), class: "commit-row-message" - else Cant find HEAD commit for this branch @@ -46,14 +46,14 @@ %td - if pipeline.started_at && pipeline.finished_at %p.duration - = icon("clock-o") + = icon("clock-o") = duration_in_numbers(pipeline.finished_at, pipeline.started_at) - if pipeline.finished_at - %p.duration + %p.finished-at = icon("calendar") #{time_ago_with_tooltip(pipeline.finished_at)} - %td + %td.pipeline-actions .controls.hidden-xs.pull-right - artifacts = pipeline.builds.latest.select { |b| b.artifacts? } - if artifacts.present? diff --git a/app/views/projects/pipelines/index.html.haml b/app/views/projects/pipelines/index.html.haml index e1c82a8179d..4672ff165d3 100644 --- a/app/views/projects/pipelines/index.html.haml +++ b/app/views/projects/pipelines/index.html.haml @@ -51,7 +51,7 @@ %th.stage %span.has-tooltip{ title: "#{stage.titleize}" } = stage.titleize - %%th + %th %th = render @pipelines, commit_sha: true, stage: true, allow_retry: true, stages: stages -- cgit v1.2.1 From 191687a10ce275f98e287876ddf7e69969790e71 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Fri, 8 Jul 2016 14:09:30 -0500 Subject: Add empty deploy dropdown button --- app/assets/stylesheets/pages/pipelines.scss | 27 +++++++++++++++++++++ .../projects/ci/pipelines/_pipeline.html.haml | 28 +++++++++++++++------- 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index fb5840a4f67..c73755ee920 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -90,12 +90,35 @@ .pipeline-actions { .btn { + margin: 0; + } + + .dropdown-toggle, + .dropdown-menu { color: $table-text-gray; + + .fa { + color: $table-text-gray; + margin-right: 6px; + font-size: 14px; + } } .btn-remove { color: $white-light; } + + .btn-group { + &.open { + .btn-default { + background-color: $white-normal; + border-color: $border-white-normal; + + &:hover { + } + } + } + } } .build-link { @@ -104,4 +127,8 @@ color: $gl-dark-link-color; } } + + .btn-group.open .dropdown-toggle { + box-shadow: none; + } } diff --git a/app/views/projects/ci/pipelines/_pipeline.html.haml b/app/views/projects/ci/pipelines/_pipeline.html.haml index b8712b5dc45..4167db3fdcb 100644 --- a/app/views/projects/ci/pipelines/_pipeline.html.haml +++ b/app/views/projects/ci/pipelines/_pipeline.html.haml @@ -57,16 +57,26 @@ .controls.hidden-xs.pull-right - artifacts = pipeline.builds.latest.select { |b| b.artifacts? } - if artifacts.present? - .dropdown.inline.build-artifacts - %button.dropdown-toggle.btn{type: 'button', 'data-toggle' => 'dropdown'} - = icon('download') - %b.caret - %ul.dropdown-menu.dropdown-menu-align-right - - artifacts.each do |build| + .btn-group.inline + .btn-group + %a.dropdown-toggle.btn.btn-default{type: 'button', 'data-toggle' => 'dropdown'} + = icon("play") + %b.caret + %ul.dropdown-menu.dropdown-menu-align-right %li - = link_to download_namespace_project_build_artifacts_path(@project.namespace, @project, build), rel: 'nofollow' do - = icon("download") - %span Download '#{build.name}' artifacts + = link_to '#' do + = icon("play") + %span Deploy to production + .btn-group + %a.dropdown-toggle.btn.btn-default.build-artifacts{type: 'button', 'data-toggle' => 'dropdown'} + = icon("download") + %b.caret + %ul.dropdown-menu.dropdown-menu-align-right + - artifacts.each do |build| + %li + = link_to download_namespace_project_build_artifacts_path(@project.namespace, @project, build), rel: 'nofollow' do + = icon("download") + %span Download '#{build.name}' artifacts - if can?(current_user, :update_pipeline, @project) - if pipeline.retryable? -- cgit v1.2.1 From a87a0ffd4429d794644c36ed84e1389ff2922895 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Fri, 8 Jul 2016 14:20:58 -0500 Subject: Add link to pipline ID --- app/views/projects/ci/pipelines/_pipeline.html.haml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/views/projects/ci/pipelines/_pipeline.html.haml b/app/views/projects/ci/pipelines/_pipeline.html.haml index 4167db3fdcb..f8c03a430bd 100644 --- a/app/views/projects/ci/pipelines/_pipeline.html.haml +++ b/app/views/projects/ci/pipelines/_pipeline.html.haml @@ -7,7 +7,8 @@ %td %div.branch-commit - %span ##{pipeline.id} + = link_to namespace_project_pipeline_path(@project.namespace, @project, pipeline.id) do + %span ##{pipeline.id} - if pipeline.ref = link_to pipeline.ref, namespace_project_commits_path(@project.namespace, @project, pipeline.ref), class: "monospace branch-name" = icon("code-fork") -- cgit v1.2.1 From 16fda5f19c2c0fb63105d7cf122360a0afaa29ca Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Mon, 11 Jul 2016 08:38:27 -0500 Subject: Update builds page --- app/assets/stylesheets/pages/pipelines.scss | 25 +++++---- app/views/projects/builds/index.html.haml | 8 +-- app/views/projects/ci/builds/_build.html.haml | 80 +++++++++++++-------------- 3 files changed, 56 insertions(+), 57 deletions(-) diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index c73755ee920..bd3dc462302 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -7,10 +7,6 @@ white-space: nowrap; } - .duration, .finished-at { - margin: 4px 0; - } - .commit-title { margin: 0; } @@ -24,9 +20,13 @@ } } -.content-list.pipelines { - width: 100%; - overflow: auto; +.content-list { + + &.pipelines, + &.builds-content-list { + width: 100%; + overflow: auto; + } } .table.builds { @@ -76,11 +76,16 @@ .avatar { margin-left: 0; } + + .label-container { + margin-top: 5px; + } } .duration, .finished-at { color: $table-text-gray; + margin: 4px 0; .fa { margin-right: 5px; @@ -91,10 +96,11 @@ .btn { margin: 0; + color: $table-text-gray; } .dropdown-toggle, - .dropdown-menu { + .dropdown-menu { color: $table-text-gray; .fa { @@ -113,9 +119,6 @@ .btn-default { background-color: $white-normal; border-color: $border-white-normal; - - &:hover { - } } } } diff --git a/app/views/projects/builds/index.html.haml b/app/views/projects/builds/index.html.haml index 381b3754cd5..85c31dfd918 100644 --- a/app/views/projects/builds/index.html.haml +++ b/app/views/projects/builds/index.html.haml @@ -36,7 +36,7 @@ = link_to ci_lint_path, class: 'btn btn-default' do %span CI Lint - %ul.content-list + %ul.content-list.builds-content-list - if @builds.blank? %li .nothing-here-block No builds to show @@ -46,14 +46,10 @@ %thead %tr %th Status - %th Build ID %th Commit - %th Ref %th Stage %th Name - %th Tags - %th Duration - %th Finished at + %th - if @project.build_coverage_enabled? %th Coverage %th diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index 00004163047..099da5d1c72 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -1,32 +1,45 @@ -%tr.build +%tr.build.commit %td.status - if can?(current_user, :read_build, build) = ci_status_with_icon(build.status, namespace_project_build_url(build.project.namespace, build.project, build)) - else = ci_status_with_icon(build.status) - %td.build-link - - if can?(current_user, :read_build, build) - = link_to namespace_project_build_url(build.project.namespace, build.project, build) do - %strong ##{build.id} - - else - %strong ##{build.id} + %td + %div.branch-commit + - if can?(current_user, :read_build, build) + = link_to namespace_project_build_url(build.project.namespace, build.project, build) do + %span ##{build.id} + - else + %span ##{build.id} - - if build.stuck? - = icon('warning', class: 'text-warning has-tooltip', title: 'Build is stuck. Check runners.') - - if defined?(retried) && retried - = icon('warning', class: 'text-warning has-tooltip', title: 'Build was retried.') + - if build.stuck? + = icon('warning', class: 'text-warning has-tooltip', title: 'Build is stuck. Check runners.') + - if defined?(retried) && retried + = icon('warning', class: 'text-warning has-tooltip', title: 'Build was retried.') - - if defined?(commit_sha) && commit_sha - %td - = link_to build.short_sha, namespace_project_commit_path(build.project.namespace, build.project, build.sha), class: "monospace" + - if defined?(ref) && ref + - if build.ref + = link_to build.ref, namespace_project_commits_path(build.project.namespace, build.project, build.ref), class: "monospace branch-name" + - else + .light none + = icon("code-fork") + + - if defined?(commit_sha) && commit_sha + = link_to build.short_sha, namespace_project_commit_path(build.project.namespace, build.project, build.sha), class: "commit-id monospace" + + .label-container + - if build.tags.any? + - build.tags.each do |tag| + %span.label.label-primary + = tag + - if build.try(:trigger_request) + %span.label.label-info triggered + - if build.try(:allow_failure) + %span.label.label-danger allowed to fail + - if defined?(retried) && retried + %span.label.label-warning retried - - if defined?(ref) && ref - %td - - if build.ref - = link_to build.ref, namespace_project_commits_path(build.project.namespace, build.project, build.ref) - - else - .light none - if defined?(runner) && runner %td @@ -43,27 +56,14 @@ = build.name %td - .label-container - - if build.tags.any? - - build.tags.each do |tag| - %span.label.label-primary - = tag - - if build.try(:trigger_request) - %span.label.label-info triggered - - if build.try(:allow_failure) - %span.label.label-danger allowed to fail - - if defined?(retried) && retried - %span.label.label-warning retried - - %td.duration - if build.duration - = icon("clock-o") - #{duration_in_words(build.finished_at, build.started_at)} - - %td.timestamp + %p.duration + = icon("clock-o") + #{duration_in_words(build.finished_at, build.started_at)} - if build.finished_at - = icon("calendar") - %span #{time_ago_with_tooltip(build.finished_at)} + %p.finished-at + = icon("calendar") + %span #{time_ago_with_tooltip(build.finished_at)} - if defined?(coverage) && coverage %td.coverage @@ -81,4 +81,4 @@ = icon('remove', class: 'cred') - elsif defined?(allow_retry) && allow_retry && build.retryable? = link_to retry_namespace_project_build_path(build.project.namespace, build.project, build, return_to: request.original_url), method: :post, title: 'Retry', class: 'btn btn-build' do - = icon('refresh') + = icon('repeat') -- cgit v1.2.1 From 694ab941ea99ca4c4a32ceea090317851390d56f Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Mon, 11 Jul 2016 13:57:12 -0500 Subject: Rearrange and update admin builds --- app/views/admin/builds/_build.html.haml | 65 +++++++++++----------- app/views/admin/builds/index.html.haml | 10 +--- app/views/projects/ci/builds/_build.html.haml | 18 +++--- .../projects/ci/pipelines/_pipeline.html.haml | 2 +- 4 files changed, 47 insertions(+), 48 deletions(-) diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml index 967151bc33b..ecf0205249a 100644 --- a/app/views/admin/builds/_build.html.haml +++ b/app/views/admin/builds/_build.html.haml @@ -1,30 +1,40 @@ - project = build.project -%tr.build +%tr.build.commit %td.status = ci_status_with_icon(build.status) - %td.build-link - - if can?(current_user, :read_build, build.project) - = link_to namespace_project_build_url(build.project.namespace, build.project, build) do - %strong Build ##{build.id} - - else - %strong Build ##{build.id} + %td + .branch-commit + - if can?(current_user, :read_build, build.project) + = link_to namespace_project_build_url(build.project.namespace, build.project, build) do + %span ##{build.id} + - else + %span ##{build.id} - - if build.stuck? - %i.fa.fa-warning.text-warning + - if build.stuck? + %i.fa.fa-warning.text-warning - %td - - if project - = link_to project.name_with_namespace, admin_namespace_project_path(project.namespace, project) + - if build.ref + = link_to build.ref, namespace_project_commits_path(build.project.namespace, build.project, build.ref), class: "monospace branch-name" + - else + .light none + = icon("code-fork") - %td - = link_to build.short_sha, namespace_project_commit_path(build.project.namespace, build.project, build.sha), class: "monospace" + = link_to build.short_sha, namespace_project_commit_path(build.project.namespace, build.project, build.sha), class: "monospace commit-id" + + - if build.tags.any? + .label-container + - build.tags.each do |tag| + %span.label.label-primary + = tag + - if build.try(:trigger_request) + %span.label.label-info triggered + - if build.try(:allow_failure) + %span.label.label-danger allowed to fail %td - - if build.ref - = link_to build.ref, namespace_project_commits_path(build.project.namespace, build.project, build.ref) - - else - .light none + - if project + = link_to project.name_with_namespace, admin_namespace_project_path(project.namespace, project) %td - if build.try(:runner) @@ -36,22 +46,15 @@ #{build.stage} / #{build.name} %td - - if build.tags.any? - - build.tags.each do |tag| - %span.label.label-primary - = tag - - if build.try(:trigger_request) - %span.label.label-info triggered - - if build.try(:allow_failure) - %span.label.label-danger allowed to fail - - %td.duration - if build.duration - #{duration_in_words(build.finished_at, build.started_at)} + %p.duration + = icon("clock-o") + #{duration_in_words(build.finished_at, build.started_at)} - %td.timestamp - if build.finished_at - %span #{time_ago_with_tooltip(build.finished_at)} + %p.finished-at + = icon("calendar") + %span #{time_ago_with_tooltip(build.finished_at)} - if defined?(coverage) && coverage %td.coverage diff --git a/app/views/admin/builds/index.html.haml b/app/views/admin/builds/index.html.haml index 1e60205f91a..9ea3cca0ecb 100644 --- a/app/views/admin/builds/index.html.haml +++ b/app/views/admin/builds/index.html.haml @@ -27,7 +27,7 @@ .row-content-block.second-block #{(@scope || 'all').capitalize} builds - %ul.content-list + %ul.content-list.builds-content-list - if @builds.blank? %li .nothing-here-block No builds to show @@ -37,15 +37,11 @@ %thead %tr %th Status - %th Build ID - %th Project %th Commit - %th Ref + %th Project %th Runner %th Name - %th Tags - %th Duration - %th Finished at + %th %th - @builds.each do |build| diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index 099da5d1c72..2099c9f8c5d 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -6,7 +6,7 @@ = ci_status_with_icon(build.status) %td - %div.branch-commit + .branch-commit - if can?(current_user, :read_build, build) = link_to namespace_project_build_url(build.project.namespace, build.project, build) do %span ##{build.id} @@ -28,17 +28,17 @@ - if defined?(commit_sha) && commit_sha = link_to build.short_sha, namespace_project_commit_path(build.project.namespace, build.project, build.sha), class: "commit-id monospace" - .label-container - - if build.tags.any? + - if build.tags.any? + .label-container - build.tags.each do |tag| %span.label.label-primary = tag - - if build.try(:trigger_request) - %span.label.label-info triggered - - if build.try(:allow_failure) - %span.label.label-danger allowed to fail - - if defined?(retried) && retried - %span.label.label-warning retried + - if build.try(:trigger_request) + %span.label.label-info triggered + - if build.try(:allow_failure) + %span.label.label-danger allowed to fail + - if defined?(retried) && retried + %span.label.label-warning retried - if defined?(runner) && runner diff --git a/app/views/projects/ci/pipelines/_pipeline.html.haml b/app/views/projects/ci/pipelines/_pipeline.html.haml index f8c03a430bd..766ea31ef70 100644 --- a/app/views/projects/ci/pipelines/_pipeline.html.haml +++ b/app/views/projects/ci/pipelines/_pipeline.html.haml @@ -6,7 +6,7 @@ %td - %div.branch-commit + .branch-commit = link_to namespace_project_pipeline_path(@project.namespace, @project, pipeline.id) do %span ##{pipeline.id} - if pipeline.ref -- cgit v1.2.1 From c57471ddb456c9640f6d77128e1fc56c7a5b35b2 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Mon, 11 Jul 2016 14:27:35 -0500 Subject: Add new stopwatch and commit icons; rename custom icon helper; fix commit pipeline layout --- app/assets/stylesheets/pages/pipelines.scss | 22 ++++++++++++++++++---- app/helpers/appearances_helper.rb | 2 +- app/views/admin/builds/_build.html.haml | 4 ++-- app/views/projects/ci/builds/_build.html.haml | 4 ++-- .../projects/ci/pipelines/_pipeline.html.haml | 4 ++-- app/views/projects/commit/_pipeline.html.haml | 4 +--- app/views/projects/issues/index.html.haml | 2 +- app/views/shared/icons/_icon_commit.svg | 3 +++ app/views/shared/icons/_icon_timer.svg | 1 + 9 files changed, 31 insertions(+), 15 deletions(-) create mode 100644 app/views/shared/icons/_icon_commit.svg create mode 100644 app/views/shared/icons/_icon_timer.svg diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index bd3dc462302..064bb83e44c 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -1,7 +1,7 @@ .pipelines { .stage { - max-width: 70px; - width: 70px; + max-width: 80px; + width: 80px; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; @@ -34,7 +34,7 @@ tr { th { - padding: 18px 10px; + padding: 16px; border: none; } } @@ -56,8 +56,11 @@ text-overflow: ellipsis; } - .fa { + svg { margin: 0 6px; + height: 14px; + width: auto; + vertical-align: middle; } .commit-id { @@ -88,6 +91,17 @@ margin: 4px 0; .fa { + font-size: 12px; + } + + svg { + height: 12px; + width: auto; + vertical-align: middle; + } + + .fa, + svg { margin-right: 5px; } } diff --git a/app/helpers/appearances_helper.rb b/app/helpers/appearances_helper.rb index 950f323e383..e12a1052988 100644 --- a/app/helpers/appearances_helper.rb +++ b/app/helpers/appearances_helper.rb @@ -31,7 +31,7 @@ module AppearancesHelper end end - def navbar_icon(icon_name, size: 16) + def custom_icon(icon_name, size: 16) render "shared/icons/#{icon_name}.svg", size: size end end diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml index ecf0205249a..c60f81f8936 100644 --- a/app/views/admin/builds/_build.html.haml +++ b/app/views/admin/builds/_build.html.haml @@ -18,7 +18,7 @@ = link_to build.ref, namespace_project_commits_path(build.project.namespace, build.project, build.ref), class: "monospace branch-name" - else .light none - = icon("code-fork") + = custom_icon("icon_commit") = link_to build.short_sha, namespace_project_commit_path(build.project.namespace, build.project, build.sha), class: "monospace commit-id" @@ -48,7 +48,7 @@ %td - if build.duration %p.duration - = icon("clock-o") + = custom_icon("icon_timer") #{duration_in_words(build.finished_at, build.started_at)} - if build.finished_at diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index 2099c9f8c5d..0c29658e4d9 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -23,7 +23,7 @@ = link_to build.ref, namespace_project_commits_path(build.project.namespace, build.project, build.ref), class: "monospace branch-name" - else .light none - = icon("code-fork") + = custom_icon("icon_commit") - if defined?(commit_sha) && commit_sha = link_to build.short_sha, namespace_project_commit_path(build.project.namespace, build.project, build.sha), class: "commit-id monospace" @@ -58,7 +58,7 @@ %td - if build.duration %p.duration - = icon("clock-o") + = custom_icon("icon_timer") #{duration_in_words(build.finished_at, build.started_at)} - if build.finished_at %p.finished-at diff --git a/app/views/projects/ci/pipelines/_pipeline.html.haml b/app/views/projects/ci/pipelines/_pipeline.html.haml index 766ea31ef70..4ef72ff5d2a 100644 --- a/app/views/projects/ci/pipelines/_pipeline.html.haml +++ b/app/views/projects/ci/pipelines/_pipeline.html.haml @@ -11,7 +11,7 @@ %span ##{pipeline.id} - if pipeline.ref = link_to pipeline.ref, namespace_project_commits_path(@project.namespace, @project, pipeline.ref), class: "monospace branch-name" - = icon("code-fork") + = custom_icon("icon_commit") = link_to pipeline.short_sha, namespace_project_commit_path(@project.namespace, @project, pipeline.sha), class: "commit-id monospace" - if pipeline.tag? %span.label.label-primary tag @@ -47,7 +47,7 @@ %td - if pipeline.started_at && pipeline.finished_at %p.duration - = icon("clock-o") + = custom_icon("icon_timer") = duration_in_numbers(pipeline.finished_at, pipeline.started_at) - if pipeline.finished_at %p.finished-at diff --git a/app/views/projects/commit/_pipeline.html.haml b/app/views/projects/commit/_pipeline.html.haml index 0411137b7c6..41fd5459429 100644 --- a/app/views/projects/commit/_pipeline.html.haml +++ b/app/views/projects/commit/_pipeline.html.haml @@ -42,9 +42,7 @@ %th Status %th Build ID %th Name - %th Tags - %th Duration - %th Finished at + %th - if pipeline.project.build_coverage_enabled? %th Coverage %th diff --git a/app/views/projects/issues/index.html.haml b/app/views/projects/issues/index.html.haml index 312bd86ed04..7612fe3719a 100644 --- a/app/views/projects/issues/index.html.haml +++ b/app/views/projects/issues/index.html.haml @@ -32,7 +32,7 @@ Code, test, and deploy together .blank-state .blank-state-icon - = navbar_icon("issues", size: 50) + = custom_icon("issues", size: 50) %h3.blank-state-title You don't have any issues right now. %p.blank-state-text diff --git a/app/views/shared/icons/_icon_commit.svg b/app/views/shared/icons/_icon_commit.svg new file mode 100644 index 00000000000..0e96035b7b7 --- /dev/null +++ b/app/views/shared/icons/_icon_commit.svg @@ -0,0 +1,3 @@ + + + diff --git a/app/views/shared/icons/_icon_timer.svg b/app/views/shared/icons/_icon_timer.svg new file mode 100644 index 00000000000..0b1e5804427 --- /dev/null +++ b/app/views/shared/icons/_icon_timer.svg @@ -0,0 +1 @@ + \ No newline at end of file -- cgit v1.2.1 From 8ed105bf42ed28be1d9c19e2cd6401fb3c89a046 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Mon, 11 Jul 2016 15:36:09 -0500 Subject: Fix label alignment bug; re-add build-link --- app/assets/stylesheets/pages/pipelines.scss | 5 ++++- app/views/admin/builds/_build.html.haml | 16 ++++++++-------- app/views/projects/ci/builds/_build.html.haml | 16 ++++++++-------- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index 064bb83e44c..cbf8297f387 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -81,7 +81,10 @@ } .label-container { - margin-top: 5px; + + .label { + margin-top: 5px; + } } } diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml index c60f81f8936..2b5a4628242 100644 --- a/app/views/admin/builds/_build.html.haml +++ b/app/views/admin/builds/_build.html.haml @@ -7,9 +7,9 @@ .branch-commit - if can?(current_user, :read_build, build.project) = link_to namespace_project_build_url(build.project.namespace, build.project, build) do - %span ##{build.id} + %span.build-link ##{build.id} - else - %span ##{build.id} + %span.build-link ##{build.id} - if build.stuck? %i.fa.fa-warning.text-warning @@ -22,15 +22,15 @@ = link_to build.short_sha, namespace_project_commit_path(build.project.namespace, build.project, build.sha), class: "monospace commit-id" - - if build.tags.any? - .label-container + .label-container + - if build.tags.any? - build.tags.each do |tag| %span.label.label-primary = tag - - if build.try(:trigger_request) - %span.label.label-info triggered - - if build.try(:allow_failure) - %span.label.label-danger allowed to fail + - if build.try(:trigger_request) + %span.label.label-info triggered + - if build.try(:allow_failure) + %span.label.label-danger allowed to fail %td - if project diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index 0c29658e4d9..f60929d0990 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -28,17 +28,17 @@ - if defined?(commit_sha) && commit_sha = link_to build.short_sha, namespace_project_commit_path(build.project.namespace, build.project, build.sha), class: "commit-id monospace" - - if build.tags.any? - .label-container + .label-container + - if build.tags.any? - build.tags.each do |tag| %span.label.label-primary = tag - - if build.try(:trigger_request) - %span.label.label-info triggered - - if build.try(:allow_failure) - %span.label.label-danger allowed to fail - - if defined?(retried) && retried - %span.label.label-warning retried + - if build.try(:trigger_request) + %span.label.label-info triggered + - if build.try(:allow_failure) + %span.label.label-danger allowed to fail + - if defined?(retried) && retried + %span.label.label-warning retried - if defined?(runner) && runner -- cgit v1.2.1 From 843ac2a351d2b957dc60ad3476faacc91d7972d8 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Mon, 11 Jul 2016 16:50:35 -0500 Subject: Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index a50edf1a797..fafbf3c7edc 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -83,6 +83,7 @@ v 8.10.0 (unreleased) - Style of import project buttons were fixed in the new project page. !5183 (rdemirbay) - Fix GitHub client requests when rate limit is disabled - Optimistic locking for Issues and Merge Requests (Title and description overriding prevention) + - Redesign Builds and Pipelines pages v 8.9.6 (unreleased) - Fix importing of events under notes for GitLab projects -- cgit v1.2.1 From 6b5cb2455e8ee9dee2b62ebba4f12e3b258f1a7c Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Mon, 11 Jul 2016 18:23:21 -0500 Subject: Update duration representation on builds pages --- app/views/admin/builds/_build.html.haml | 2 +- app/views/projects/ci/builds/_build.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml index 2b5a4628242..ce818c30c30 100644 --- a/app/views/admin/builds/_build.html.haml +++ b/app/views/admin/builds/_build.html.haml @@ -49,7 +49,7 @@ - if build.duration %p.duration = custom_icon("icon_timer") - #{duration_in_words(build.finished_at, build.started_at)} + = duration_in_numbers(build.finished_at, build.started_at) - if build.finished_at %p.finished-at diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index f60929d0990..e1b42b2cfa5 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -59,7 +59,7 @@ - if build.duration %p.duration = custom_icon("icon_timer") - #{duration_in_words(build.finished_at, build.started_at)} + = duration_in_numbers(build.finished_at, build.started_at) - if build.finished_at %p.finished-at = icon("calendar") -- cgit v1.2.1 From bd7d6124524e0a2222f7837b27857b363b34729f Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 12 Jul 2016 14:55:18 -0500 Subject: Update CHANGELOG for 8.9.6 [ci skip] --- CHANGELOG | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index a50edf1a797..6fbbec0a520 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -84,8 +84,11 @@ v 8.10.0 (unreleased) - Fix GitHub client requests when rate limit is disabled - Optimistic locking for Issues and Merge Requests (Title and description overriding prevention) -v 8.9.6 (unreleased) - - Fix importing of events under notes for GitLab projects +v 8.9.6 + - Fix importing of events under notes for GitLab projects. !5154 + - Fix log statements in import/export. !5129 + - Fix commit avatar alignment in compare view. !5128 + - Fix broken migration in MySQL. !5005 v 8.9.5 - Add more debug info to import/export and memory killer. !5108 -- cgit v1.2.1 From 0ec69815868f77526bd5a2c060d22fbe272af9ea Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 11 Jul 2016 19:48:35 -0500 Subject: Use number_with_delimiter for Todos pending/done tab counts --- app/views/dashboard/todos/index.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/dashboard/todos/index.html.haml b/app/views/dashboard/todos/index.html.haml index fc42e5dcc66..4e340b6ec16 100644 --- a/app/views/dashboard/todos/index.html.haml +++ b/app/views/dashboard/todos/index.html.haml @@ -9,14 +9,14 @@ %span To do %span.badge - = todos_pending_count + = number_with_delimiter(todos_pending_count) - todo_done_active = ('active' if params[:state] == 'done') %li{class: "todos-done #{todo_done_active}"} = link_to todos_filter_path(state: 'done') do %span Done %span.badge - = todos_done_count + = number_with_delimiter(todos_done_count) .nav-controls - if @todos.any?(&:pending?) -- cgit v1.2.1 From 3404e10fd003efdc44d9689eec120babd0513009 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Tue, 12 Jul 2016 15:58:26 -0500 Subject: Change running status color to blue; update icon to spinner --- app/assets/stylesheets/pages/merge_requests.scss | 7 +++++-- app/assets/stylesheets/pages/status.scss | 12 +++++++++--- app/helpers/ci_status_helper.rb | 4 +++- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index d9756b66af0..15c6c9f231a 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -73,11 +73,14 @@ color: #888; } - &.ci-pending, - &.ci-running { + &.ci-pending { color: $gl-warning; } + &.ci-running { + color: $blue-normal; + } + &.ci-failed, &.ci-error { color: $gl-danger; diff --git a/app/assets/stylesheets/pages/status.scss b/app/assets/stylesheets/pages/status.scss index 2370d35924e..c6b053150be 100644 --- a/app/assets/stylesheets/pages/status.scss +++ b/app/assets/stylesheets/pages/status.scss @@ -32,11 +32,15 @@ border-color: $gl-gray; } - &.ci-pending, - &.ci-running { + &.ci-pending { color: $gl-warning; border-color: $gl-warning; } + + &.ci-running { + color: $blue-normal; + border-color: $blue-normal; + } } .ci-status-icon-success { @@ -45,10 +49,12 @@ .ci-status-icon-failed { color: $gl-danger; } - .ci-status-icon-running, .ci-status-icon-pending { color: $gl-warning; } + .ci-status-icon-running { + color: $blue-normal; + } .ci-status-icon-canceled, .ci-status-icon-disabled, .ci-status-icon-not-found, diff --git a/app/helpers/ci_status_helper.rb b/app/helpers/ci_status_helper.rb index 8e4ae1e6aec..e6c99c9959e 100644 --- a/app/helpers/ci_status_helper.rb +++ b/app/helpers/ci_status_helper.rb @@ -29,8 +29,10 @@ module CiStatusHelper 'check' when 'failed' 'close' - when 'running', 'pending' + when 'pending' 'clock-o' + when 'running' + 'spinner' else 'circle' end -- cgit v1.2.1 From 6434be6384c34c0edd377bea11247100a65192ba Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Tue, 12 Jul 2016 16:12:23 -0500 Subject: Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 3a176cd5a1e..bdc2c6aae28 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -84,6 +84,7 @@ v 8.10.0 (unreleased) - Fix GitHub client requests when rate limit is disabled - Optimistic locking for Issues and Merge Requests (Title and description overriding prevention) - Redesign Builds and Pipelines pages + - Change status color and icon for running builds v 8.9.6 - Fix importing of events under notes for GitLab projects. !5154 -- cgit v1.2.1 From 489e1937043d6f58af28d11831ba6da94e90a705 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 13 Jul 2016 00:02:10 -0500 Subject: Rename constant to be more descriptive --- lib/gitlab/diff/inline_diff.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/diff/inline_diff.rb b/lib/gitlab/diff/inline_diff.rb index 4a3eb92b9fc..28ad637fda4 100644 --- a/lib/gitlab/diff/inline_diff.rb +++ b/lib/gitlab/diff/inline_diff.rb @@ -2,7 +2,7 @@ module Gitlab module Diff class InlineDiff # Regex to find a run of deleted lines followed by the same number of added lines - REGEX = %r{ + LINE_PAIRS_PATTERN = %r{ # Runs start at the beginning of the string (the first line) or after a space (for an unchanged line) (?:\A|\s) @@ -72,7 +72,7 @@ module Gitlab line_prefixes = lines.each_with_object("") { |line, s| s << line[0] }.gsub(/[^ +-]/, ' ') changed_line_pairs = [] - line_prefixes.scan(REGEX) do + line_prefixes.scan(LINE_PAIRS_PATTERN) do # For `"---+++"`, `begin_index == 0`, `end_index == 6` begin_index, end_index = Regexp.last_match.offset(:del_ins) -- cgit v1.2.1 From f0577d838544152f558411ef1101d56c5852d92e Mon Sep 17 00:00:00 2001 From: Mathias Vestergaard Date: Fri, 20 May 2016 01:58:34 +0200 Subject: Added "developers can merge" setting to protected branches - Cherry-picked from `mvestergaard:branch-protection-dev-merge` - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4220 --- CHANGELOG | 1 + app/assets/javascripts/protected_branches.js.coffee | 9 ++++++--- .../projects/protected_branches_controller.rb | 2 +- app/models/merge_request.rb | 3 ++- app/models/project.rb | 4 ++++ app/services/git_push_service.rb | 3 ++- .../protected_branches/_branches_list.html.haml | 2 ++ .../protected_branches/_protected_branch.html.haml | 2 ++ .../projects/protected_branches/index.html.haml | 8 ++++++++ ...dd_developers_can_merge_to_protected_branches.rb | 9 +++++++++ db/schema.rb | 7 ++++--- lib/gitlab/access.rb | 8 +++++--- lib/gitlab/git_access.rb | 10 ++++++++++ spec/lib/gitlab/git_access_spec.rb | 21 +++++++++++++++++++++ spec/services/git_push_service_spec.rb | 13 +++++++++++-- 15 files changed, 88 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb diff --git a/CHANGELOG b/CHANGELOG index f1dcc40a273..e144ba8513f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -497,6 +497,7 @@ v 8.7.7 - Prevent unauthorized access to other projects build traces - Forbid scripting for wiki files - Only show notes through JSON on confidential issues that the user has access to + - Added protected branch setting that allows Developers to accept merge requests while push is still disallowed. !4220 (Mathias Vestergaard) v 8.7.6 - Fix links on wiki pages for relative url setups. !4131 (Artem Sidorenko) diff --git a/app/assets/javascripts/protected_branches.js.coffee b/app/assets/javascripts/protected_branches.js.coffee index 79c2306e4d2..ce2fc883620 100644 --- a/app/assets/javascripts/protected_branches.js.coffee +++ b/app/assets/javascripts/protected_branches.js.coffee @@ -1,9 +1,11 @@ $ -> $(".protected-branches-list :checkbox").change (e) -> name = $(this).attr("name") - if name == "developers_can_push" + row = $(this).parents("tr") + if name == "developers_can_push" || name == "developers_can_merge" id = $(this).val() - checked = $(this).is(":checked") + can_push = row.find("input[name=developers_can_push]").is(":checked") + can_merge = row.find("input[name=developers_can_merge]").is(":checked") url = $(this).data("url") $.ajax type: "PUT" @@ -12,7 +14,8 @@ $ -> data: id: id protected_branch: - developers_can_push: checked + developers_can_push: can_push + developers_can_merge: can_merge success: -> row = $(e.target) diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb index 80dad758afa..10dca47fded 100644 --- a/app/controllers/projects/protected_branches_controller.rb +++ b/app/controllers/projects/protected_branches_controller.rb @@ -50,6 +50,6 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController end def protected_branch_params - params.require(:protected_branch).permit(:name, :developers_can_push) + params.require(:protected_branch).permit(:name, :developers_can_push, :developers_can_merge) end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 157901378d3..23d68706527 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -552,7 +552,8 @@ class MergeRequest < ActiveRecord::Base end def can_be_merged_by?(user) - ::Gitlab::GitAccess.new(user, project, 'web').can_push_to_branch?(target_branch) + access = ::Gitlab::GitAccess.new(user, project, 'web') + access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch) end def mergeable_ci_state? diff --git a/app/models/project.rb b/app/models/project.rb index a66b750cd48..2bd97fc48f1 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -832,6 +832,10 @@ class Project < ActiveRecord::Base protected_branches.matching(branch_name).any?(&:developers_can_push) end + def developers_can_merge_to_protected_branch?(branch_name) + protected_branches.matching(branch_name).any?(&:developers_can_merge) + end + def forked? !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?) end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index a886f35981f..e02b50ff9a2 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -89,7 +89,8 @@ class GitPushService < BaseService # Set protection on the default branch if configured if current_application_settings.default_branch_protection != PROTECTION_NONE developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false - @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push }) + developers_can_merge = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? true : false + @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push, developers_can_merge: developers_can_merge }) end end diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml index 97cb1a9052b..181d1a27c73 100644 --- a/app/views/projects/protected_branches/_branches_list.html.haml +++ b/app/views/projects/protected_branches/_branches_list.html.haml @@ -8,6 +8,7 @@ .table-responsive %table.table.protected-branches-list %colgroup + %col{ width: "27%" } %col{ width: "30%" } %col{ width: "25%" } %col{ width: "25%" } @@ -18,6 +19,7 @@ %th Protected Branch %th Commit %th Developers Can Push + %th Developers can Merge - if can_admin_project %th %tbody diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml index 474aec3a97c..7fda7f96047 100644 --- a/app/views/projects/protected_branches/_protected_branch.html.haml +++ b/app/views/projects/protected_branches/_protected_branch.html.haml @@ -16,6 +16,8 @@ (branch was removed from repository) %td = check_box_tag("developers_can_push", protected_branch.id, protected_branch.developers_can_push, data: { url: url }) + %td + = check_box_tag("developers_can_merge", protected_branch.id, protected_branch.developers_can_merge, data: { url: url }) - if can_admin_project %td = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right" diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml index 3fab95751e0..101b3f3b452 100644 --- a/app/views/projects/protected_branches/index.html.haml +++ b/app/views/projects/protected_branches/index.html.haml @@ -36,6 +36,14 @@ = f.label :developers_can_push, "Developers can push", class: "label-light append-bottom-0" %p.light.append-bottom-0 Allow developers to push to this branch + + .form-group + = f.check_box :developers_can_merge, class: "pull-left" + .prepend-left-20 + = f.label :developers_can_merge, "Developers can merge", class: "label-light append-bottom-0" + %p.light.append-bottom-0 + Allow developers to accept merge requests to this branch = f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true + %hr = render "branches_list" diff --git a/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb b/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb new file mode 100644 index 00000000000..15ad8e8bcbb --- /dev/null +++ b/db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb @@ -0,0 +1,9 @@ +class AddDevelopersCanMergeToProtectedBranches < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def change + add_column_with_default :protected_branches, :developers_can_merge, :boolean, default: false, allow_null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index f24e47b85b2..4562e6bb0c3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -860,11 +860,12 @@ ActiveRecord::Schema.define(version: 20160712171823) do add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree create_table "protected_branches", force: :cascade do |t| - t.integer "project_id", null: false - t.string "name", null: false + t.integer "project_id", null: false + t.string "name", null: false t.datetime "created_at" t.datetime "updated_at" - t.boolean "developers_can_push", default: false, null: false + t.boolean "developers_can_push", default: false, null: false + t.boolean "developers_can_merge", default: false, null: false end add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index 831f1e635ba..de41ea415a6 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -14,9 +14,10 @@ module Gitlab OWNER = 50 # Branch protection settings - PROTECTION_NONE = 0 - PROTECTION_DEV_CAN_PUSH = 1 - PROTECTION_FULL = 2 + PROTECTION_NONE = 0 + PROTECTION_DEV_CAN_PUSH = 1 + PROTECTION_FULL = 2 + PROTECTION_DEV_CAN_MERGE = 3 class << self def values @@ -54,6 +55,7 @@ module Gitlab def protection_options { "Not protected: Both developers and masters can push new commits, force push, or delete the branch." => PROTECTION_NONE, + "Protected against pushes: Developers cannot push new commits, but are allowed to accept merge requests to the branch." => PROTECTION_DEV_CAN_MERGE, "Partially protected: Developers can push new commits, but cannot force push or delete the branch. Masters can do all of those." => PROTECTION_DEV_CAN_PUSH, "Fully protected: Developers cannot push new commits, force push, or delete the branch. Only masters can do any of those." => PROTECTION_FULL, } diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 7679c7e4bb8..e20e3338262 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -39,6 +39,16 @@ module Gitlab end end + def can_merge_to_branch?(ref) + return false unless user + + if project.protected_branch?(ref) && !project.developers_can_merge_to_protected_branch?(ref) + user.can?(:push_code_to_protected_branches, project) + else + user.can?(:push_code, project) + end + end + def can_read_project? if user user.can?(:read_project, project) diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index c79ba11f782..b90d9c724f1 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -65,6 +65,27 @@ describe Gitlab::GitAccess, lib: true do expect(access.can_push_to_branch?(@branch.name)).to be_falsey end end + + describe 'merge to protected branch if allowed for developers' do + before do + @branch = create :protected_branch, project: project, developers_can_merge: true + end + + it "returns true if user is a master" do + project.team << [user, :master] + expect(access.can_merge_to_branch?(@branch.name)).to be_truthy + end + + it "returns true if user is a developer" do + project.team << [user, :developer] + expect(access.can_merge_to_branch?(@branch.name)).to be_truthy + end + + it "returns false if user is a reporter" do + project.team << [user, :reporter] + expect(access.can_merge_to_branch?(@branch.name)).to be_falsey + end + end end describe '#check with single protocols allowed' do diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index afabeed4a80..0f30a84a2cf 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -224,7 +224,7 @@ describe GitPushService, services: true do it "when pushing a branch for the first time" do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false }) + expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: false }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end @@ -242,7 +242,16 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true }) + expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true, developers_can_merge: false }) + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) + end + + it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + + expect(project).to receive(:execute_hooks) + expect(project.default_branch).to eq("master") + expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: true }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end -- cgit v1.2.1 From 495db09653bafb0371e5d5a5f12d5bc33cdb584b Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 23 Jun 2016 14:58:14 +0530 Subject: Enforce "developers can merge" during `pre-receive`. 1. When a merge request is being merged, save the merge commit SHA in the `in_progress_merge_commit_sha` database column. 2. The `pre-receive` hook looks for any locked (in progress) merge request with `in_progress_merge_commit_sha` matching the `newrev` it is passed. 3. If it finds a matching MR, the merge is legitimate. 4. Update `git_access_spec` to test the behaviour we added here. Also refactored this spec a bit to make it easier to add more contexts / conditions. --- app/models/repository.rb | 14 +- app/services/merge_requests/merge_service.rb | 4 +- ..._progress_merge_commit_sha_to_merge_requests.rb | 8 + db/schema.rb | 1 + lib/gitlab/checks/matching_merge_request.rb | 18 ++ lib/gitlab/git_access.rb | 7 + spec/lib/gitlab/diff/position_tracer_spec.rb | 3 +- spec/lib/gitlab/git_access_spec.rb | 185 +++++++++++++-------- spec/models/repository_spec.rb | 9 +- .../merge_requests/refresh_service_spec.rb | 3 +- 10 files changed, 167 insertions(+), 85 deletions(-) create mode 100644 db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb create mode 100644 lib/gitlab/checks/matching_merge_request.rb diff --git a/app/models/repository.rb b/app/models/repository.rb index 5b670cb4b8f..09487b62f98 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -769,9 +769,9 @@ class Repository end end - def merge(user, source_sha, target_branch, options = {}) - our_commit = rugged.branches[target_branch].target - their_commit = rugged.lookup(source_sha) + def merge(user, merge_request, options = {}) + our_commit = rugged.branches[merge_request.target_branch].target + their_commit = rugged.lookup(merge_request.diff_head_sha) raise "Invalid merge target" if our_commit.nil? raise "Invalid merge source" if their_commit.nil? @@ -779,14 +779,16 @@ class Repository merge_index = rugged.merge_commits(our_commit, their_commit) return false if merge_index.conflicts? - commit_with_hooks(user, target_branch) do |ref| + commit_with_hooks(user, merge_request.target_branch) do |tmp_ref| actual_options = options.merge( parents: [our_commit, their_commit], tree: merge_index.write_tree(rugged), - update_ref: ref + update_ref: tmp_ref ) - Rugged::Commit.create(rugged, actual_options) + commit_id = Rugged::Commit.create(rugged, actual_options) + merge_request.update(in_progress_merge_commit_sha: commit_id) + commit_id end end diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index f1b1d90c457..0dac0614141 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -34,7 +34,7 @@ module MergeRequests committer: committer } - commit_id = repository.merge(current_user, merge_request.diff_head_sha, merge_request.target_branch, options) + commit_id = repository.merge(current_user, merge_request, options) merge_request.update(merge_commit_sha: commit_id) rescue GitHooksService::PreReceiveError => e merge_request.update(merge_error: e.message) @@ -43,6 +43,8 @@ module MergeRequests merge_request.update(merge_error: "Something went wrong during merge") Rails.logger.error(e.message) false + ensure + merge_request.update(in_progress_merge_commit_sha: nil) end def after_merge diff --git a/db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb b/db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb new file mode 100644 index 00000000000..7c5f76572ef --- /dev/null +++ b/db/migrate/20160629025435_add_column_in_progress_merge_commit_sha_to_merge_requests.rb @@ -0,0 +1,8 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddColumnInProgressMergeCommitShaToMergeRequests < ActiveRecord::Migration + def change + add_column :merge_requests, :in_progress_merge_commit_sha, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 4562e6bb0c3..64019cf79bb 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -626,6 +626,7 @@ ActiveRecord::Schema.define(version: 20160712171823) do t.string "merge_commit_sha" t.datetime "deleted_at" t.integer "lock_version", default: 0, null: false + t.string "in_progress_merge_commit_sha" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/lib/gitlab/checks/matching_merge_request.rb b/lib/gitlab/checks/matching_merge_request.rb new file mode 100644 index 00000000000..849848515da --- /dev/null +++ b/lib/gitlab/checks/matching_merge_request.rb @@ -0,0 +1,18 @@ +module Gitlab + module Checks + class MatchingMergeRequest + def initialize(newrev, branch_name, project) + @newrev = newrev + @branch_name = branch_name + @project = project + end + + def match? + @project.merge_requests + .with_state(:locked) + .where(in_progress_merge_commit_sha: @newrev, target_branch: @branch_name) + .exists? + end + end + end +end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index e20e3338262..feaf845209e 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -177,10 +177,15 @@ module Gitlab Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev) end + def protocol_allowed? Gitlab::ProtocolAccess.allowed?(protocol) end + def matching_merge_request?(newrev, branch_name) + Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? + end + private def protected_branch_action(oldrev, newrev, branch_name) @@ -190,6 +195,8 @@ module Gitlab elsif Gitlab::Git.blank_ref?(newrev) # and we dont allow remove of protected branch :remove_protected_branches + elsif matching_merge_request?(newrev, branch_name) && project.developers_can_merge_to_protected_branch?(branch_name) + :push_code elsif project.developers_can_push_to_protected_branch?(branch_name) :push_code else diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb index 08312e60f4a..c268f84c759 100644 --- a/spec/lib/gitlab/diff/position_tracer_spec.rb +++ b/spec/lib/gitlab/diff/position_tracer_spec.rb @@ -1639,7 +1639,8 @@ describe Gitlab::Diff::PositionTracer, lib: true do committer: committer } - repository.merge(current_user, second_create_file_commit.sha, branch_name, options) + merge_request = create(:merge_request, source_branch: second_create_file_commit.sha, target_branch: branch_name, source_project: project) + repository.merge(current_user, merge_request, options) project.commit(branch_name) end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index b90d9c724f1..50cf2f1e93f 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -181,96 +181,47 @@ describe Gitlab::GitAccess, lib: true do end describe 'push_access_check' do - def protect_feature_branch - create(:protected_branch, name: 'feature', project: project) - end + before { merge_into_protected_branch } + let(:unprotected_branch) { FFaker::Internet.user_name } - def changes - { - push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow", + let(:changes) do + { push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow", push_master: '6f6d7e7ed 570e7b2ab refs/heads/master', push_protected_branch: '6f6d7e7ed 570e7b2ab refs/heads/feature', push_remove_protected_branch: "570e7b2ab #{Gitlab::Git::BLANK_SHA} "\ 'refs/heads/feature', push_tag: '6f6d7e7ed 570e7b2ab refs/tags/v1.0.0', push_new_tag: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/tags/v7.8.9", - push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature'] - } - end - - def self.permissions_matrix - { - master: { - push_new_branch: true, - push_master: true, - push_protected_branch: true, - push_remove_protected_branch: false, - push_tag: true, - push_new_tag: true, - push_all: true, - }, - - developer: { - push_new_branch: true, - push_master: true, - push_protected_branch: false, - push_remove_protected_branch: false, - push_tag: false, - push_new_tag: true, - push_all: false, - }, - - reporter: { - push_new_branch: false, - push_master: false, - push_protected_branch: false, - push_remove_protected_branch: false, - push_tag: false, - push_new_tag: false, - push_all: false, - }, - - guest: { - push_new_branch: false, - push_master: false, - push_protected_branch: false, - push_remove_protected_branch: false, - push_tag: false, - push_new_tag: false, - push_all: false, - } - } + push_all: ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature'], + merge_into_protected_branch: "0b4bc9a #{merge_into_protected_branch} refs/heads/feature" } end - def self.updated_permissions_matrix - updated_permissions_matrix = permissions_matrix.dup - updated_permissions_matrix[:developer][:push_protected_branch] = true - updated_permissions_matrix[:developer][:push_all] = true - updated_permissions_matrix + def stub_git_hooks + # Running the `pre-receive` hook is expensive, and not necessary for this test. + allow_any_instance_of(GitHooksService).to receive(:execute).and_yield end - permissions_matrix.keys.each do |role| - describe "#{role} access" do - before { protect_feature_branch } - before { project.team << [user, role] } + def merge_into_protected_branch + @protected_branch_merge_commit ||= begin + stub_git_hooks + project.repository.add_branch(user, unprotected_branch, 'feature') + target_branch = project.repository.lookup('feature') + source_branch = project.repository.commit_file(user, FFaker::InternetSE.login_user_name, FFaker::HipsterIpsum.paragraph, FFaker::HipsterIpsum.sentence, unprotected_branch, false) + rugged = project.repository.rugged + author = { email: "email@example.com", time: Time.now, name: "Example Git User" } - permissions_matrix[role].each do |action, allowed| - context action do - subject { access.push_access_check(changes[action]) } - it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey } - end - end + merge_index = rugged.merge_commits(target_branch, source_branch) + Rugged::Commit.create(rugged, author: author, committer: author, message: "commit message", parents: [target_branch, source_branch], tree: merge_index.write_tree(rugged)) end end - context "with enabled developers push to protected branches " do - updated_permissions_matrix.keys.each do |role| + def self.run_permission_checks(permissions_matrix) + permissions_matrix.keys.each do |role| describe "#{role} access" do - before { create(:protected_branch, name: 'feature', developers_can_push: true, project: project) } before { project.team << [user, role] } - updated_permissions_matrix[role].each do |action, allowed| + permissions_matrix[role].each do |action, allowed| context action do subject { access.push_access_check(changes[action]) } @@ -280,5 +231,97 @@ describe Gitlab::GitAccess, lib: true do end end end + + permissions_matrix = { + master: { + push_new_branch: true, + push_master: true, + push_protected_branch: true, + push_remove_protected_branch: false, + push_tag: true, + push_new_tag: true, + push_all: true, + merge_into_protected_branch: true + }, + + developer: { + push_new_branch: true, + push_master: true, + push_protected_branch: false, + push_remove_protected_branch: false, + push_tag: false, + push_new_tag: true, + push_all: false, + merge_into_protected_branch: false + }, + + reporter: { + push_new_branch: false, + push_master: false, + push_protected_branch: false, + push_remove_protected_branch: false, + push_tag: false, + push_new_tag: false, + push_all: false, + merge_into_protected_branch: false + }, + + guest: { + push_new_branch: false, + push_master: false, + push_protected_branch: false, + push_remove_protected_branch: false, + push_tag: false, + push_new_tag: false, + push_all: false, + merge_into_protected_branch: false + } + } + + [['feature', 'exact'], ['feat*', 'wildcard']].each do |protected_branch_name, protected_branch_type| + context do + before { create(:protected_branch, name: protected_branch_name, project: project) } + + run_permission_checks(permissions_matrix) + end + + context "when 'developers can push' is turned on for the #{protected_branch_type} protected branch" do + before { create(:protected_branch, name: protected_branch_name, developers_can_push: true, project: project) } + + run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true })) + end + + context "when 'developers can merge' is turned on for the #{protected_branch_type} protected branch" do + before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, project: project) } + + context "when a merge request exists for the given source/target branch" do + context "when the merge request is in progress" do + before do + create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch) + end + + run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true })) + end + + context "when the merge request is not in progress" do + before do + create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', in_progress_merge_commit_sha: nil) + end + + run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false })) + end + end + + context "when a merge request does not exist for the given source/target branch" do + run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false })) + end + end + + context "when 'developers can merge' and 'developers can push' are turned on for the #{protected_branch_type} protected branch" do + before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, developers_can_push: true, project: project) } + + run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true })) + end + end end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index b39b958450c..e14cec589fe 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -4,16 +4,17 @@ describe Repository, models: true do include RepoHelpers TestBlob = Struct.new(:name) - let(:repository) { create(:project).repository } + let(:project) { create(:project) } + let(:repository) { project.repository } let(:user) { create(:user) } let(:commit_options) do author = repository.user_to_committer(user) { message: 'Test message', committer: author, author: author } end let(:merge_commit) do - source_sha = repository.find_branch('feature').target - merge_commit_sha = repository.merge(user, source_sha, 'master', commit_options) - repository.commit(merge_commit_sha) + merge_request = create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project) + merge_commit_id = repository.merge(user, merge_request, commit_options) + repository.commit(merge_commit_id) end describe '#branch_names_contains' do diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 06f56d85aa8..ce643b3f860 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -88,8 +88,7 @@ describe MergeRequests::RefreshService, services: true do # Merge master -> feature branch author = { email: 'test@gitlab.com', time: Time.now, name: "Me" } commit_options = { message: 'Test message', committer: author, author: author } - master_commit = @project.repository.commit('master') - @project.repository.merge(@user, master_commit.id, 'feature', commit_options) + @project.repository.merge(@user, @merge_request, commit_options) commit = @project.repository.commit('feature') service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature') reload_mrs -- cgit v1.2.1 From 60245bbe228014a9f689adafd64b571883ef6eb3 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 24 Jun 2016 10:37:04 +0530 Subject: Refactor `Gitlab::GitAccess` 1. Don't use case statements for dispatch anymore. This leads to a lot of duplication, and makes the logic harder to follow. 2. Remove duplicated logic. - For example, the `can_push_to_branch?` exists, but we also have a different way of checking the same condition within `change_access_check`. - This kind of duplication is removed, and the `can_push_to_branch?` method is used in both places. 3. Move checks returning true/false to `UserAccess`. - All public methods in `GitAccess` now return an instance of `GitAccessStatus`. Previously, some methods would return true/false as well, which was confusing. - It makes sense for these kinds of checks to be at the level of a user, so the `UserAccess` class was repurposed for this. The prior `UserAccess.allowed?` classmethod is converted into an instance method. - All external uses of these checks have been migrated to use the `UserAccess` class 4. Move the "change_access_check" into a separate class. - Create the `GitAccess::ChangeAccessCheck` class to run these checks, which are quite substantial. - `ChangeAccessCheck` returns an instance of `GitAccessStatus` as well. 5. Break out the boolean logic in `ChangeAccessCheck` into `if/else` chains - this seems more readable. 6. I can understand that this might look like overkill for !4892, but I think this is a good opportunity to clean it up. - http://martinfowler.com/bliki/OpportunisticRefactoring.html --- app/helpers/branches_helper.rb | 2 +- app/models/merge_request.rb | 2 +- app/services/commits/change_service.rb | 4 +- app/services/files/base_service.rb | 2 +- lib/api/helpers.rb | 2 +- lib/gitlab/git_access.rb | 159 +++++---------------------- lib/gitlab/git_access/change_access_check.rb | 96 ++++++++++++++++ lib/gitlab/git_access_wiki.rb | 2 +- lib/gitlab/user_access.rb | 48 +++++++- spec/lib/gitlab/git_access_spec.rb | 82 -------------- spec/lib/gitlab/user_access_spec.rb | 90 +++++++++++++++ spec/requests/api/api_helpers_spec.rb | 4 +- 12 files changed, 270 insertions(+), 223 deletions(-) create mode 100644 lib/gitlab/git_access/change_access_check.rb create mode 100644 spec/lib/gitlab/user_access_spec.rb diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb index 601df5c18df..bfd23aa4e04 100644 --- a/app/helpers/branches_helper.rb +++ b/app/helpers/branches_helper.rb @@ -12,7 +12,7 @@ module BranchesHelper def can_push_branch?(project, branch_name) return false unless project.repository.branch_exists?(branch_name) - ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(branch_name) + ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch_name) end def project_branches diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 23d68706527..e5853bdd589 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -552,7 +552,7 @@ class MergeRequest < ActiveRecord::Base end def can_be_merged_by?(user) - access = ::Gitlab::GitAccess.new(user, project, 'web') + access = ::Gitlab::UserAccess.new(user, project: project) access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch) end diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb index c578097376a..ed73d8cb8c2 100644 --- a/app/services/commits/change_service.rb +++ b/app/services/commits/change_service.rb @@ -23,7 +23,7 @@ module Commits private def check_push_permissions - allowed = ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(@target_branch) + allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch) unless allowed raise ValidationError.new('You are not allowed to push into this branch') @@ -31,7 +31,7 @@ module Commits true end - + def create_target_branch(new_branch) # Temporary branch exists and contains the change commit return success if repository.find_branch(new_branch) diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index 37c5e321b39..55da949f56a 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -42,7 +42,7 @@ module Files end def validate - allowed = ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(@target_branch) + allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch) unless allowed raise_error("You are not allowed to push into this branch") diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 77e407b54c5..73557cf7db6 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -17,7 +17,7 @@ module API def current_user @current_user ||= (find_user_by_private_token || doorkeeper_guard) - unless @current_user && Gitlab::UserAccess.allowed?(@current_user) + unless @current_user && Gitlab::UserAccess.new(@current_user).allowed? return nil end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index feaf845209e..cfb48ac2382 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -1,62 +1,17 @@ +# Check a user's access to perform a git action. All public methods in this +# class return an instance of `GitlabAccessStatus` module Gitlab class GitAccess DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive } PUSH_COMMANDS = %w{ git-receive-pack } - attr_reader :actor, :project, :protocol + attr_reader :actor, :project, :protocol, :user_access def initialize(actor, project, protocol) @actor = actor @project = project @protocol = protocol - end - - def user - return @user if defined?(@user) - - @user = - case actor - when User - actor - when DeployKey - nil - when Key - actor.user - end - end - - def deploy_key - actor if actor.is_a?(DeployKey) - end - - def can_push_to_branch?(ref) - return false unless user - - if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref) - user.can?(:push_code_to_protected_branches, project) - else - user.can?(:push_code, project) - end - end - - def can_merge_to_branch?(ref) - return false unless user - - if project.protected_branch?(ref) && !project.developers_can_merge_to_protected_branch?(ref) - user.can?(:push_code_to_protected_branches, project) - else - user.can?(:push_code, project) - end - end - - def can_read_project? - if user - user.can?(:read_project, project) - elsif deploy_key - deploy_key.projects.include?(project) - else - false - end + @user_access = UserAccess.new(user, project: project) end def check(cmd, changes = nil) @@ -66,11 +21,11 @@ module Gitlab return build_status_object(false, "No user or key was provided.") end - if user && !user_allowed? + if user && !user_access.allowed? return build_status_object(false, "Your account has been blocked.") end - unless project && can_read_project? + unless project && (user_access.can_read_project? || deploy_key_can_read_project?) return build_status_object(false, 'The project you were looking for could not be found.') end @@ -105,7 +60,7 @@ module Gitlab end def user_download_access_check - unless user.can?(:download_code, project) + unless user_access.can_do_action?(:download_code) return build_status_object(false, "You are not allowed to download code from this project.") end @@ -135,103 +90,49 @@ module Gitlab build_status_object(true) end - def can_user_do_action?(action) - @permission_cache ||= {} - @permission_cache[action] ||= user.can?(action, project) - end - def change_access_check(change) - oldrev, newrev, ref = change.split(' ') - - action = - if project.protected_branch?(branch_name(ref)) - protected_branch_action(oldrev, newrev, branch_name(ref)) - elsif (tag_ref = tag_name(ref)) && protected_tag?(tag_ref) - # Prevent any changes to existing git tag unless user has permissions - :admin_project - else - :push_code - end - - unless can_user_do_action?(action) - status = - case action - when :force_push_code_to_protected_branches - build_status_object(false, "You are not allowed to force push code to a protected branch on this project.") - when :remove_protected_branches - build_status_object(false, "You are not allowed to deleted protected branches from this project.") - when :push_code_to_protected_branches - build_status_object(false, "You are not allowed to push code to protected branches on this project.") - when :admin_project - build_status_object(false, "You are not allowed to change existing tags on this project.") - else # :push_code - build_status_object(false, "You are not allowed to push code to this project.") - end - return status - end - - build_status_object(true) - end - - def forced_push?(oldrev, newrev) - Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev) + ChangeAccessCheck.new(change, user_access: user_access, project: project).exec end - def protocol_allowed? Gitlab::ProtocolAccess.allowed?(protocol) end - def matching_merge_request?(newrev, branch_name) - Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? - end - private - def protected_branch_action(oldrev, newrev, branch_name) - # we dont allow force push to protected branch - if forced_push?(oldrev, newrev) - :force_push_code_to_protected_branches - elsif Gitlab::Git.blank_ref?(newrev) - # and we dont allow remove of protected branch - :remove_protected_branches - elsif matching_merge_request?(newrev, branch_name) && project.developers_can_merge_to_protected_branch?(branch_name) - :push_code - elsif project.developers_can_push_to_protected_branch?(branch_name) - :push_code - else - :push_code_to_protected_branches - end - end - - def protected_tag?(tag_name) - project.repository.tag_exists?(tag_name) + def matching_merge_request?(newrev, branch_name) + Checks::MatchingMergeRequest.new(newrev, branch_name, project).match? end - def user_allowed? - Gitlab::UserAccess.allowed?(user) + def deploy_key + actor if actor.is_a?(DeployKey) end - def branch_name(ref) - ref = ref.to_s - if Gitlab::Git.branch_ref?(ref) - Gitlab::Git.ref_name(ref) - else - nil - end - end - def tag_name(ref) - ref = ref.to_s - if Gitlab::Git.tag_ref?(ref) - Gitlab::Git.ref_name(ref) + def deploy_key_can_read_project? + if deploy_key + deploy_key.projects.include?(project) else - nil + false end end protected + def user + return @user if defined?(@user) + + @user = + case actor + when User + actor + when DeployKey + nil + when Key + actor.user + end + end + def build_status_object(status, message = '') GitAccessStatus.new(status, message) end diff --git a/lib/gitlab/git_access/change_access_check.rb b/lib/gitlab/git_access/change_access_check.rb new file mode 100644 index 00000000000..9268da4ec6e --- /dev/null +++ b/lib/gitlab/git_access/change_access_check.rb @@ -0,0 +1,96 @@ +module Gitlab + class GitAccess + class ChangeAccessCheck + attr_reader :user_access, :project + + def initialize(change, user_access:, project:) + @oldrev, @newrev, @ref = change.split(' ') + @branch_name = branch_name(@ref) + @user_access = user_access + @project = project + end + + def exec + error = protected_branch_checks || tag_checks || push_checks + + if error + GitAccessStatus.new(false, error) + else + GitAccessStatus.new(true) + end + end + + protected + + def protected_branch_checks + return unless project.protected_branch?(@branch_name) + + return unless project.protected_branch?(@branch_name) + + if forced_push? && user_access.cannot_do_action?(:force_push_code_to_protected_branches) + return "You are not allowed to force push code to a protected branch on this project." + elsif Gitlab::Git.blank_ref?(@newrev) && user_access.cannot_do_action?(:remove_protected_branches) + return "You are not allowed to deleted protected branches from this project." + end + + if matching_merge_request? + if user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name) + return + else + "You are not allowed to merge code into protected branches on this project." + end + else + if user_access.can_push_to_branch?(@branch_name) + return + else + "You are not allowed to push code to protected branches on this project." + end + end + end + + def tag_checks + if (tag_ref = tag_name(@ref)) && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project) + "You are not allowed to change existing tags on this project." + end + end + + def push_checks + if user_access.cannot_do_action?(:push_code) + "You are not allowed to push code to this project." + end + end + + private + + def protected_tag?(tag_name) + project.repository.tag_exists?(tag_name) + end + + def forced_push? + Gitlab::ForcePushCheck.force_push?(@project, @oldrev, @newrev) + end + + def matching_merge_request? + Checks::MatchingMergeRequest.new(@newrev, @branch_name, @project).match? + end + + def branch_name(ref) + ref = @ref.to_s + if Gitlab::Git.branch_ref?(ref) + Gitlab::Git.ref_name(ref) + else + nil + end + end + + def tag_name(ref) + ref = @ref.to_s + if Gitlab::Git.tag_ref?(ref) + Gitlab::Git.ref_name(ref) + else + nil + end + end + end + end +end diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index 8672cbc0ec4..f71d3575909 100644 --- a/lib/gitlab/git_access_wiki.rb +++ b/lib/gitlab/git_access_wiki.rb @@ -1,7 +1,7 @@ module Gitlab class GitAccessWiki < GitAccess def change_access_check(change) - if user.can?(:create_wiki, project) + if user_access.can_do_action?(:create_wiki) build_status_object(true) else build_status_object(false, "You are not allowed to write to this project's wiki.") diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index d1b42c1f9b9..c0f85e9b3a8 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -1,7 +1,23 @@ module Gitlab - module UserAccess - def self.allowed?(user) - return false if user.blocked? + class UserAccess + attr_reader :user, :project + + def initialize(user, project: nil) + @user = user + @project = project + end + + def can_do_action?(action) + @permission_cache ||= {} + @permission_cache[action] ||= user.can?(action, project) + end + + def cannot_do_action?(action) + !can_do_action?(action) + end + + def allowed? + return false if user.blank? || user.blocked? if user.requires_ldap_check? && user.try_obtain_ldap_lease return false unless Gitlab::LDAP::Access.allowed?(user) @@ -9,5 +25,31 @@ module Gitlab true end + + def can_push_to_branch?(ref) + return false unless user + + if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref) + user.can?(:push_code_to_protected_branches, project) + else + user.can?(:push_code, project) + end + end + + def can_merge_to_branch?(ref) + return false unless user + + if project.protected_branch?(ref) && !project.developers_can_merge_to_protected_branch?(ref) + user.can?(:push_code_to_protected_branches, project) + else + user.can?(:push_code, project) + end + end + + def can_read_project? + return false unless user + + user.can?(:read_project, project) + end end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 50cf2f1e93f..059d8a2c355 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -6,88 +6,6 @@ describe Gitlab::GitAccess, lib: true do let(:user) { create(:user) } let(:actor) { user } - describe 'can_push_to_branch?' do - describe 'push to none protected branch' do - it "returns true if user is a master" do - project.team << [user, :master] - expect(access.can_push_to_branch?("random_branch")).to be_truthy - end - - it "returns true if user is a developer" do - project.team << [user, :developer] - expect(access.can_push_to_branch?("random_branch")).to be_truthy - end - - it "returns false if user is a reporter" do - project.team << [user, :reporter] - expect(access.can_push_to_branch?("random_branch")).to be_falsey - end - end - - describe 'push to protected branch' do - before do - @branch = create :protected_branch, project: project - end - - it "returns true if user is a master" do - project.team << [user, :master] - expect(access.can_push_to_branch?(@branch.name)).to be_truthy - end - - it "returns false if user is a developer" do - project.team << [user, :developer] - expect(access.can_push_to_branch?(@branch.name)).to be_falsey - end - - it "returns false if user is a reporter" do - project.team << [user, :reporter] - expect(access.can_push_to_branch?(@branch.name)).to be_falsey - end - end - - describe 'push to protected branch if allowed for developers' do - before do - @branch = create :protected_branch, project: project, developers_can_push: true - end - - it "returns true if user is a master" do - project.team << [user, :master] - expect(access.can_push_to_branch?(@branch.name)).to be_truthy - end - - it "returns true if user is a developer" do - project.team << [user, :developer] - expect(access.can_push_to_branch?(@branch.name)).to be_truthy - end - - it "returns false if user is a reporter" do - project.team << [user, :reporter] - expect(access.can_push_to_branch?(@branch.name)).to be_falsey - end - end - - describe 'merge to protected branch if allowed for developers' do - before do - @branch = create :protected_branch, project: project, developers_can_merge: true - end - - it "returns true if user is a master" do - project.team << [user, :master] - expect(access.can_merge_to_branch?(@branch.name)).to be_truthy - end - - it "returns true if user is a developer" do - project.team << [user, :developer] - expect(access.can_merge_to_branch?(@branch.name)).to be_truthy - end - - it "returns false if user is a reporter" do - project.team << [user, :reporter] - expect(access.can_merge_to_branch?(@branch.name)).to be_falsey - end - end - end - describe '#check with single protocols allowed' do def disable_protocol(protocol) settings = ::ApplicationSetting.create_from_defaults diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb new file mode 100644 index 00000000000..c77b746cc88 --- /dev/null +++ b/spec/lib/gitlab/user_access_spec.rb @@ -0,0 +1,90 @@ +require 'spec_helper' + +describe Gitlab::UserAccess, lib: true do + let(:access) { Gitlab::UserAccess.new(user, project: project) } + let(:project) { create(:project) } + let(:user) { create(:user) } + + describe 'can_push_to_branch?' do + describe 'push to none protected branch' do + it "returns true if user is a master" do + project.team << [user, :master] + expect(access.can_push_to_branch?("random_branch")).to be_truthy + end + + it "returns true if user is a developer" do + project.team << [user, :developer] + expect(access.can_push_to_branch?("random_branch")).to be_truthy + end + + it "returns false if user is a reporter" do + project.team << [user, :reporter] + expect(access.can_push_to_branch?("random_branch")).to be_falsey + end + end + + describe 'push to protected branch' do + before do + @branch = create :protected_branch, project: project + end + + it "returns true if user is a master" do + project.team << [user, :master] + expect(access.can_push_to_branch?(@branch.name)).to be_truthy + end + + it "returns false if user is a developer" do + project.team << [user, :developer] + expect(access.can_push_to_branch?(@branch.name)).to be_falsey + end + + it "returns false if user is a reporter" do + project.team << [user, :reporter] + expect(access.can_push_to_branch?(@branch.name)).to be_falsey + end + end + + describe 'push to protected branch if allowed for developers' do + before do + @branch = create :protected_branch, project: project, developers_can_push: true + end + + it "returns true if user is a master" do + project.team << [user, :master] + expect(access.can_push_to_branch?(@branch.name)).to be_truthy + end + + it "returns true if user is a developer" do + project.team << [user, :developer] + expect(access.can_push_to_branch?(@branch.name)).to be_truthy + end + + it "returns false if user is a reporter" do + project.team << [user, :reporter] + expect(access.can_push_to_branch?(@branch.name)).to be_falsey + end + end + + describe 'merge to protected branch if allowed for developers' do + before do + @branch = create :protected_branch, project: project, developers_can_merge: true + end + + it "returns true if user is a master" do + project.team << [user, :master] + expect(access.can_merge_to_branch?(@branch.name)).to be_truthy + end + + it "returns true if user is a developer" do + project.team << [user, :developer] + expect(access.can_merge_to_branch?(@branch.name)).to be_truthy + end + + it "returns false if user is a reporter" do + project.team << [user, :reporter] + expect(access.can_merge_to_branch?(@branch.name)).to be_falsey + end + end + + end +end diff --git a/spec/requests/api/api_helpers_spec.rb b/spec/requests/api/api_helpers_spec.rb index 83025953889..3d5c19aeff3 100644 --- a/spec/requests/api/api_helpers_spec.rb +++ b/spec/requests/api/api_helpers_spec.rb @@ -49,7 +49,7 @@ describe API::Helpers, api: true do it "should return nil for a user without access" do env[API::Helpers::PRIVATE_TOKEN_HEADER] = user.private_token - allow(Gitlab::UserAccess).to receive(:allowed?).and_return(false) + allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false) expect(current_user).to be_nil end @@ -73,7 +73,7 @@ describe API::Helpers, api: true do it "should return nil for a user without access" do env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token - allow(Gitlab::UserAccess).to receive(:allowed?).and_return(false) + allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false) expect(current_user).to be_nil end -- cgit v1.2.1 From 959d63ab1a5559f5f60ba7378a59e2d0fdb17c73 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 24 Jun 2016 11:13:02 +0530 Subject: Clean up `protected_branches.js.coffee` - Only send a param for the currently changed checkbox. - Have the controller use strong parameters correctly, so that the PATCH works as expected. --- app/assets/javascripts/protected_branches.js.coffee | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/protected_branches.js.coffee b/app/assets/javascripts/protected_branches.js.coffee index ce2fc883620..14afef2e2ee 100644 --- a/app/assets/javascripts/protected_branches.js.coffee +++ b/app/assets/javascripts/protected_branches.js.coffee @@ -1,21 +1,18 @@ $ -> $(".protected-branches-list :checkbox").change (e) -> name = $(this).attr("name") - row = $(this).parents("tr") if name == "developers_can_push" || name == "developers_can_merge" id = $(this).val() - can_push = row.find("input[name=developers_can_push]").is(":checked") - can_merge = row.find("input[name=developers_can_merge]").is(":checked") + can_push = $(this).is(":checked") url = $(this).data("url") $.ajax - type: "PUT" + type: "PATCH" url: url dataType: "json" data: id: id protected_branch: - developers_can_push: can_push - developers_can_merge: can_merge + "#{name}": can_push success: -> row = $(e.target) -- cgit v1.2.1 From af7e75162efe07e9fcb1186462e56bd2325018de Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 4 Jul 2016 11:06:32 +0530 Subject: Don't ask the user to "merge this request manually". 1. If they are a developer with "Developers can Merge" switched on. --- app/models/merge_request.rb | 5 +++++ app/views/projects/merge_requests/widget/open/_conflicts.html.haml | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e5853bdd589..471e32f3b60 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -556,6 +556,11 @@ class MergeRequest < ActiveRecord::Base access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch) end + def can_be_merged_via_command_line_by?(user) + access = ::Gitlab::UserAccess.new(user, project: project) + access.can_push_to_branch?(target_branch) + end + def mergeable_ci_state? return true unless project.only_allow_merge_if_build_succeeds? diff --git a/app/views/projects/merge_requests/widget/open/_conflicts.html.haml b/app/views/projects/merge_requests/widget/open/_conflicts.html.haml index 06ab0a3fa00..f000cc38a65 100644 --- a/app/views/projects/merge_requests/widget/open/_conflicts.html.haml +++ b/app/views/projects/merge_requests/widget/open/_conflicts.html.haml @@ -4,7 +4,7 @@ %p Please resolve these conflicts or - - if @merge_request.can_be_merged_by?(current_user) + - if @merge_request.can_be_merged_via_command_line_by?(current_user) #{link_to "merge this request manually", "#modal_merge_info", class: "how_to_merge_link vlink", "data-toggle" => "modal"}. - else ask someone with write access to this repository to merge this request manually. -- cgit v1.2.1 From dffdc7b27c1f89e4e26b4714834e4f147c656206 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 4 Jul 2016 11:07:37 +0530 Subject: Add "Developers can Merge" (to protected branches) to the CHANGELOG. --- CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index e144ba8513f..b0d86e4acad 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -43,6 +43,7 @@ v 8.10.0 (unreleased) - Add "Enabled Git access protocols" to Application Settings - Diffs will create button/diff form on demand no on server side - Reduce size of HTML used by diff comment forms + - Protected branches have a "Developers can Merge" setting. !4892 (original implementation by Mathias Vestergaard) - Fix user creation with stronger minimum password requirements !4054 (nathan-pmt) - Only show New Snippet button to users that can create snippets. - PipelinesFinder uses git cache data @@ -497,7 +498,6 @@ v 8.7.7 - Prevent unauthorized access to other projects build traces - Forbid scripting for wiki files - Only show notes through JSON on confidential issues that the user has access to - - Added protected branch setting that allows Developers to accept merge requests while push is still disallowed. !4220 (Mathias Vestergaard) v 8.7.6 - Fix links on wiki pages for relative url setups. !4131 (Artem Sidorenko) -- cgit v1.2.1 From 4d00ed21ebbc9fd4a1f1b13cbed9a0a9ad2a2a9e Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 5 Jul 2016 08:55:46 +0530 Subject: Appease rubocop. --- lib/gitlab/git_access.rb | 1 - spec/lib/gitlab/git_access_spec.rb | 1 - 2 files changed, 2 deletions(-) diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index cfb48ac2382..9f5bb9d62cd 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -108,7 +108,6 @@ module Gitlab actor if actor.is_a?(DeployKey) end - def deploy_key_can_read_project? if deploy_key deploy_key.projects.include?(project) diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 059d8a2c355..db33c7a22bb 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -128,7 +128,6 @@ describe Gitlab::GitAccess, lib: true do rugged = project.repository.rugged author = { email: "email@example.com", time: Time.now, name: "Example Git User" } - merge_index = rugged.merge_commits(target_branch, source_branch) Rugged::Commit.create(rugged, author: author, committer: author, message: "commit message", parents: [target_branch, source_branch], tree: merge_index.write_tree(rugged)) end -- cgit v1.2.1 From ea9e8f4609f46f9c5713a8346f91dc19d310c2e1 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 7 Jul 2016 11:36:01 +0530 Subject: Move all "checks" under `GitLab::Checks`. - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4892#note_12892160 - This is more consistent. --- app/services/merge_requests/refresh_service.rb | 2 +- lib/gitlab/checks/change_access.rb | 94 +++++++++++++++++++++++++ lib/gitlab/checks/force_push.rb | 17 +++++ lib/gitlab/force_push_check.rb | 15 ---- lib/gitlab/git_access.rb | 2 +- lib/gitlab/git_access/change_access_check.rb | 96 -------------------------- 6 files changed, 113 insertions(+), 113 deletions(-) create mode 100644 lib/gitlab/checks/change_access.rb create mode 100644 lib/gitlab/checks/force_push.rb delete mode 100644 lib/gitlab/force_push_check.rb delete mode 100644 lib/gitlab/git_access/change_access_check.rb diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index b11ecd97a57..1daf6bbf553 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -48,7 +48,7 @@ module MergeRequests end def force_push? - Gitlab::ForcePushCheck.force_push?(@project, @oldrev, @newrev) + Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev) end # Refresh merge request diff if we push to source or target branch of merge request diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb new file mode 100644 index 00000000000..cb8f4f2cbdd --- /dev/null +++ b/lib/gitlab/checks/change_access.rb @@ -0,0 +1,94 @@ +module Gitlab + module Checks + class ChangeAccess + attr_reader :user_access, :project + + def initialize(change, user_access:, project:) + @oldrev, @newrev, @ref = change.split(' ') + @branch_name = branch_name(@ref) + @user_access = user_access + @project = project + end + + def exec + error = protected_branch_checks || tag_checks || push_checks + + if error + GitAccessStatus.new(false, error) + else + GitAccessStatus.new(true) + end + end + + protected + + def protected_branch_checks + return unless project.protected_branch?(@branch_name) + + if forced_push? && user_access.cannot_do_action?(:force_push_code_to_protected_branches) + return "You are not allowed to force push code to a protected branch on this project." + elsif Gitlab::Git.blank_ref?(@newrev) && user_access.cannot_do_action?(:remove_protected_branches) + return "You are not allowed to deleted protected branches from this project." + end + + if matching_merge_request? + if user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name) + return + else + "You are not allowed to merge code into protected branches on this project." + end + else + if user_access.can_push_to_branch?(@branch_name) + return + else + "You are not allowed to push code to protected branches on this project." + end + end + end + + def tag_checks + if (tag_ref = tag_name(@ref)) && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project) + "You are not allowed to change existing tags on this project." + end + end + + def push_checks + if user_access.cannot_do_action?(:push_code) + "You are not allowed to push code to this project." + end + end + + private + + def protected_tag?(tag_name) + project.repository.tag_exists?(tag_name) + end + + def forced_push? + Gitlab::Checks::ForcePush.force_push?(@project, @oldrev, @newrev) + end + + def matching_merge_request? + Checks::MatchingMergeRequest.new(@newrev, @branch_name, @project).match? + end + + def branch_name(ref) + ref = @ref.to_s + if Gitlab::Git.branch_ref?(ref) + Gitlab::Git.ref_name(ref) + else + nil + end + end + + def tag_name(ref) + ref = @ref.to_s + if Gitlab::Git.tag_ref?(ref) + Gitlab::Git.ref_name(ref) + else + nil + end + end + end + end +end diff --git a/lib/gitlab/checks/force_push.rb b/lib/gitlab/checks/force_push.rb new file mode 100644 index 00000000000..dfa83a0eab3 --- /dev/null +++ b/lib/gitlab/checks/force_push.rb @@ -0,0 +1,17 @@ +module Gitlab + module Checks + class ForcePush + def self.force_push?(project, oldrev, newrev) + return false if project.empty_repo? + + # Created or deleted branch + if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev) + false + else + missed_refs, _ = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} --git-dir=#{project.repository.path_to_repo} rev-list #{oldrev} ^#{newrev})) + missed_refs.split("\n").size > 0 + end + end + end + end +end diff --git a/lib/gitlab/force_push_check.rb b/lib/gitlab/force_push_check.rb deleted file mode 100644 index 93c6a5bb7f5..00000000000 --- a/lib/gitlab/force_push_check.rb +++ /dev/null @@ -1,15 +0,0 @@ -module Gitlab - class ForcePushCheck - def self.force_push?(project, oldrev, newrev) - return false if project.empty_repo? - - # Created or deleted branch - if Gitlab::Git.blank_ref?(oldrev) || Gitlab::Git.blank_ref?(newrev) - false - else - missed_refs, _ = Gitlab::Popen.popen(%W(#{Gitlab.config.git.bin_path} --git-dir=#{project.repository.path_to_repo} rev-list #{oldrev} ^#{newrev})) - missed_refs.split("\n").size > 0 - end - end - end -end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 9f5bb9d62cd..308f23bc9bc 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -91,7 +91,7 @@ module Gitlab end def change_access_check(change) - ChangeAccessCheck.new(change, user_access: user_access, project: project).exec + Checks::ChangeAccess.new(change, user_access: user_access, project: project).exec end def protocol_allowed? diff --git a/lib/gitlab/git_access/change_access_check.rb b/lib/gitlab/git_access/change_access_check.rb deleted file mode 100644 index 9268da4ec6e..00000000000 --- a/lib/gitlab/git_access/change_access_check.rb +++ /dev/null @@ -1,96 +0,0 @@ -module Gitlab - class GitAccess - class ChangeAccessCheck - attr_reader :user_access, :project - - def initialize(change, user_access:, project:) - @oldrev, @newrev, @ref = change.split(' ') - @branch_name = branch_name(@ref) - @user_access = user_access - @project = project - end - - def exec - error = protected_branch_checks || tag_checks || push_checks - - if error - GitAccessStatus.new(false, error) - else - GitAccessStatus.new(true) - end - end - - protected - - def protected_branch_checks - return unless project.protected_branch?(@branch_name) - - return unless project.protected_branch?(@branch_name) - - if forced_push? && user_access.cannot_do_action?(:force_push_code_to_protected_branches) - return "You are not allowed to force push code to a protected branch on this project." - elsif Gitlab::Git.blank_ref?(@newrev) && user_access.cannot_do_action?(:remove_protected_branches) - return "You are not allowed to deleted protected branches from this project." - end - - if matching_merge_request? - if user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name) - return - else - "You are not allowed to merge code into protected branches on this project." - end - else - if user_access.can_push_to_branch?(@branch_name) - return - else - "You are not allowed to push code to protected branches on this project." - end - end - end - - def tag_checks - if (tag_ref = tag_name(@ref)) && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project) - "You are not allowed to change existing tags on this project." - end - end - - def push_checks - if user_access.cannot_do_action?(:push_code) - "You are not allowed to push code to this project." - end - end - - private - - def protected_tag?(tag_name) - project.repository.tag_exists?(tag_name) - end - - def forced_push? - Gitlab::ForcePushCheck.force_push?(@project, @oldrev, @newrev) - end - - def matching_merge_request? - Checks::MatchingMergeRequest.new(@newrev, @branch_name, @project).match? - end - - def branch_name(ref) - ref = @ref.to_s - if Gitlab::Git.branch_ref?(ref) - Gitlab::Git.ref_name(ref) - else - nil - end - end - - def tag_name(ref) - ref = @ref.to_s - if Gitlab::Git.tag_ref?(ref) - Gitlab::Git.ref_name(ref) - else - nil - end - end - end - end -end -- cgit v1.2.1 From bb81f2afc1d82d6e88d7039025c8a8be0794aac6 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 12 Jul 2016 10:47:36 +0530 Subject: Implement last round of review comments from !4892. 1. Fix typos, minor styling errors. 2. Use single quotes rather than double quotes in `user_access_spec`. 3. Test formatting. --- .../protected_branches/_branches_list.html.haml | 4 +-- lib/gitlab/checks/change_access.rb | 6 ++-- spec/lib/gitlab/user_access_spec.rb | 40 ++++++++++------------ spec/services/git_push_service_spec.rb | 3 +- 4 files changed, 27 insertions(+), 26 deletions(-) diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml index 181d1a27c73..720d67dff7c 100644 --- a/app/views/projects/protected_branches/_branches_list.html.haml +++ b/app/views/projects/protected_branches/_branches_list.html.haml @@ -8,7 +8,7 @@ .table-responsive %table.table.protected-branches-list %colgroup - %col{ width: "27%" } + %col{ width: "20%" } %col{ width: "30%" } %col{ width: "25%" } %col{ width: "25%" } @@ -19,7 +19,7 @@ %th Protected Branch %th Commit %th Developers Can Push - %th Developers can Merge + %th Developers Can Merge - if can_admin_project %th %tbody diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index cb8f4f2cbdd..5551fac4b8b 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -28,7 +28,7 @@ module Gitlab if forced_push? && user_access.cannot_do_action?(:force_push_code_to_protected_branches) return "You are not allowed to force push code to a protected branch on this project." elsif Gitlab::Git.blank_ref?(@newrev) && user_access.cannot_do_action?(:remove_protected_branches) - return "You are not allowed to deleted protected branches from this project." + return "You are not allowed to delete protected branches from this project." end if matching_merge_request? @@ -47,7 +47,9 @@ module Gitlab end def tag_checks - if (tag_ref = tag_name(@ref)) && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project) + tag_ref = tag_name(@ref) + + if tag_ref && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project) "You are not allowed to change existing tags on this project." end end diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index c77b746cc88..aa9ec243498 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -7,40 +7,38 @@ describe Gitlab::UserAccess, lib: true do describe 'can_push_to_branch?' do describe 'push to none protected branch' do - it "returns true if user is a master" do + it 'returns true if user is a master' do project.team << [user, :master] - expect(access.can_push_to_branch?("random_branch")).to be_truthy + expect(access.can_push_to_branch?('random_branch')).to be_truthy end - it "returns true if user is a developer" do + it 'returns true if user is a developer' do project.team << [user, :developer] - expect(access.can_push_to_branch?("random_branch")).to be_truthy + expect(access.can_push_to_branch?('random_branch')).to be_truthy end - it "returns false if user is a reporter" do + it 'returns false if user is a reporter' do project.team << [user, :reporter] - expect(access.can_push_to_branch?("random_branch")).to be_falsey + expect(access.can_push_to_branch?('random_branch')).to be_falsey end end describe 'push to protected branch' do - before do - @branch = create :protected_branch, project: project - end + let(:branch) { create :protected_branch, project: project } - it "returns true if user is a master" do + it 'returns true if user is a master' do project.team << [user, :master] - expect(access.can_push_to_branch?(@branch.name)).to be_truthy + expect(access.can_push_to_branch?(branch.name)).to be_truthy end - it "returns false if user is a developer" do + it 'returns false if user is a developer' do project.team << [user, :developer] - expect(access.can_push_to_branch?(@branch.name)).to be_falsey + expect(access.can_push_to_branch?(branch.name)).to be_falsey end - it "returns false if user is a reporter" do + it 'returns false if user is a reporter' do project.team << [user, :reporter] - expect(access.can_push_to_branch?(@branch.name)).to be_falsey + expect(access.can_push_to_branch?(branch.name)).to be_falsey end end @@ -49,17 +47,17 @@ describe Gitlab::UserAccess, lib: true do @branch = create :protected_branch, project: project, developers_can_push: true end - it "returns true if user is a master" do + it 'returns true if user is a master' do project.team << [user, :master] expect(access.can_push_to_branch?(@branch.name)).to be_truthy end - it "returns true if user is a developer" do + it 'returns true if user is a developer' do project.team << [user, :developer] expect(access.can_push_to_branch?(@branch.name)).to be_truthy end - it "returns false if user is a reporter" do + it 'returns false if user is a reporter' do project.team << [user, :reporter] expect(access.can_push_to_branch?(@branch.name)).to be_falsey end @@ -70,17 +68,17 @@ describe Gitlab::UserAccess, lib: true do @branch = create :protected_branch, project: project, developers_can_merge: true end - it "returns true if user is a master" do + it 'returns true if user is a master' do project.team << [user, :master] expect(access.can_merge_to_branch?(@branch.name)).to be_truthy end - it "returns true if user is a developer" do + it 'returns true if user is a developer' do project.team << [user, :developer] expect(access.can_merge_to_branch?(@branch.name)).to be_truthy end - it "returns false if user is a reporter" do + it 'returns false if user is a reporter' do project.team << [user, :reporter] expect(access.can_merge_to_branch?(@branch.name)).to be_falsey end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 0f30a84a2cf..47c0580e0f0 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -243,7 +243,8 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true, developers_can_merge: false }) - execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) + + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master') end it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do -- cgit v1.2.1 From cc752f241ccef3b13486544f03ae716de00cc363 Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Wed, 13 Jul 2016 09:23:55 +0200 Subject: ObjectRenderer doesn't crash when no objects to cache with Rails.cache.read_multi --- lib/banzai/renderer.rb | 14 ++++++++++---- spec/lib/banzai/object_renderer_spec.rb | 11 +++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/lib/banzai/renderer.rb b/lib/banzai/renderer.rb index ea10b06439b..910687a7b6a 100644 --- a/lib/banzai/renderer.rb +++ b/lib/banzai/renderer.rb @@ -61,10 +61,16 @@ module Banzai end cacheable_items, non_cacheable_items = items_collection.partition { |item| item.key?(:cache_key) } - items_in_cache = Rails.cache.read_multi(*cacheable_items.map { |item| item[:cache_key] }) - items_not_in_cache = cacheable_items.reject do |item| - item[:rendered] = items_in_cache[item[:cache_key]] - items_in_cache.key?(item[:cache_key]) + + items_in_cache = [] + items_not_in_cache = [] + + unless cacheable_items.empty? + items_in_cache = Rails.cache.read_multi(*cacheable_items.map { |item| item[:cache_key] }) + items_not_in_cache = cacheable_items.reject do |item| + item[:rendered] = items_in_cache[item[:cache_key]] + items_in_cache.key?(item[:cache_key]) + end end (items_not_in_cache + non_cacheable_items).each do |item| diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb index cf6cdd33ebb..bcdb95250ca 100644 --- a/spec/lib/banzai/object_renderer_spec.rb +++ b/spec/lib/banzai/object_renderer_spec.rb @@ -109,6 +109,17 @@ describe Banzai::ObjectRenderer do expect(docs[1]).to be_an_instance_of(Nokogiri::HTML::DocumentFragment) expect(docs[1].to_html).to eq('

bye

') end + + it 'returns when no objects to render' do + objects = [] + renderer = described_class.new(project, user, pipeline: :note) + + expect(Banzai).to receive(:cache_collection_render). + with([]). + and_call_original + + expect(renderer.render_attributes(objects, :note)).to eq([]) + end end describe '#base_context' do -- cgit v1.2.1