From 292bca0546c59b9816c696371cd9bbf04ba19fb2 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 30 Sep 2015 15:38:21 -0400 Subject: [PATCH 01/27] Only allow password reset emails once per minute Addresses internal https://dev.gitlab.org/gitlab/gitlabhq/issues/2611 --- app/controllers/passwords_controller.rb | 22 +++++++++---- spec/features/password_reset_spec.rb | 43 +++++++++++++++++++++---- 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index edf43935f3..a2d152addc 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -2,18 +2,19 @@ class PasswordsController < Devise::PasswordsController def create email = resource_params[:email] - resource_found = resource_class.find_by_email(email) - if resource_found && resource_found.ldap_user? + self.resource = resource_class.find_by_email(email) + + if resource && resource.ldap_user? flash[:alert] = "Cannot reset password for LDAP user." respond_with({}, location: after_sending_reset_password_instructions_path_for(resource_name)) and return end - self.resource = resource_class.send_reset_password_instructions(resource_params) - if successfully_sent?(resource) - respond_with({}, location: after_sending_reset_password_instructions_path_for(resource_name)) - else - respond_with(resource) + unless can_send_reset_email? + flash[:alert] = "Instructions about how to reset your password have already been sent recently. Please wait a few minutes to try again." + respond_with({}, location: new_password_path(resource_name)) and return end + + super end def edit @@ -35,4 +36,11 @@ class PasswordsController < Devise::PasswordsController end end end + + private + + def can_send_reset_email? + resource && (resource.reset_password_sent_at.blank? || + resource.reset_password_sent_at < 1.minute.ago) + end end diff --git a/spec/features/password_reset_spec.rb b/spec/features/password_reset_spec.rb index abf66f2356..ce7a66a0da 100644 --- a/spec/features/password_reset_spec.rb +++ b/spec/features/password_reset_spec.rb @@ -1,13 +1,44 @@ require 'spec_helper' feature 'Password reset', feature: true do - describe 'with two-factor authentication' do - let(:user) { create(:user, :two_factor) } + describe 'throttling' do + it 'sends reset instructions when not previously sent' do + visit root_path + forgot_password(create(:user)) + expect(page).to have_content(I18n.t('devise.passwords.send_instructions')) + expect(current_path).to eq new_user_session_path + end + + it 'sends reset instructions when previously sent more than a minute ago' do + user = create(:user) + user.send_reset_password_instructions + user.update_attribute(:reset_password_sent_at, 5.minutes.ago) + + visit root_path + forgot_password(user) + + expect(page).to have_content(I18n.t('devise.passwords.send_instructions')) + expect(current_path).to eq new_user_session_path + end + + it "throttles multiple resets in a short timespan" do + user = create(:user) + user.send_reset_password_instructions + + visit root_path + forgot_password(user) + + expect(page).to have_content("Instructions about how to reset your password have already been sent recently. Please wait a few minutes to try again.") + expect(current_path).to eq new_user_password_path + end + end + + describe 'with two-factor authentication' do it 'requires login after password reset' do visit root_path - forgot_password + forgot_password(create(:user, :two_factor)) reset_password expect(page).to have_content("Your password was changed successfully.") @@ -17,12 +48,10 @@ feature 'Password reset', feature: true do end describe 'without two-factor authentication' do - let(:user) { create(:user) } - it 'requires login after password reset' do visit root_path - forgot_password + forgot_password(create(:user)) reset_password expect(page).to have_content("Your password was changed successfully.") @@ -30,7 +59,7 @@ feature 'Password reset', feature: true do end end - def forgot_password + def forgot_password(user) click_on 'Forgot your password?' fill_in 'Email', with: user.email click_button 'Reset password' From 5c80ceee0d3545061f1ca85ad8c25d0583e1e4d2 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 30 Sep 2015 15:39:59 -0400 Subject: [PATCH 02/27] Autofocus the email field on the password reset form --- app/views/devise/passwords/new.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/devise/passwords/new.html.haml b/app/views/devise/passwords/new.html.haml index 29ffe8a8be..535e85869e 100644 --- a/app/views/devise/passwords/new.html.haml +++ b/app/views/devise/passwords/new.html.haml @@ -6,7 +6,7 @@ .devise-errors = devise_error_messages! .clearfix.append-bottom-20 - = f.email_field :email, placeholder: "Email", class: "form-control", required: true, value: params[:user_email] + = f.email_field :email, placeholder: "Email", class: "form-control", required: true, value: params[:user_email], autofocus: true .clearfix = f.submit "Reset password", class: "btn-primary btn" From ceb21cc49f27ddfade4d28e7ad8805d481706922 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 30 Sep 2015 15:47:48 -0400 Subject: [PATCH 03/27] Update feature spec --- spec/features/password_reset_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/features/password_reset_spec.rb b/spec/features/password_reset_spec.rb index ce7a66a0da..deb90a4450 100644 --- a/spec/features/password_reset_spec.rb +++ b/spec/features/password_reset_spec.rb @@ -55,6 +55,7 @@ feature 'Password reset', feature: true do reset_password expect(page).to have_content("Your password was changed successfully.") + expect(page).not_to have_content("You are now signed in.") expect(current_path).to eq new_user_session_path end end From 9052f13b31944cc1c69af3dec8176fde0bb080a6 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 30 Sep 2015 16:15:56 -0400 Subject: [PATCH 04/27] Remove specs for "login after reset" We're now using default Devise behavior, so these tests were redundant. --- spec/features/password_reset_spec.rb | 40 ---------------------------- 1 file changed, 40 deletions(-) diff --git a/spec/features/password_reset_spec.rb b/spec/features/password_reset_spec.rb index deb90a4450..4d512c6543 100644 --- a/spec/features/password_reset_spec.rb +++ b/spec/features/password_reset_spec.rb @@ -34,50 +34,10 @@ feature 'Password reset', feature: true do end end - describe 'with two-factor authentication' do - it 'requires login after password reset' do - visit root_path - - forgot_password(create(:user, :two_factor)) - reset_password - - expect(page).to have_content("Your password was changed successfully.") - expect(page).not_to have_content("You are now signed in.") - expect(current_path).to eq new_user_session_path - end - end - - describe 'without two-factor authentication' do - it 'requires login after password reset' do - visit root_path - - forgot_password(create(:user)) - reset_password - - expect(page).to have_content("Your password was changed successfully.") - expect(page).not_to have_content("You are now signed in.") - expect(current_path).to eq new_user_session_path - end - end - def forgot_password(user) click_on 'Forgot your password?' fill_in 'Email', with: user.email click_button 'Reset password' user.reload end - - def get_reset_token - mail = ActionMailer::Base.deliveries.last - body = mail.body.encoded - body.scan(/reset_password_token=(.+)\"/).flatten.first - end - - def reset_password(password = 'password') - visit edit_user_password_path(reset_password_token: get_reset_token) - - fill_in 'New password', with: password - fill_in 'Confirm new password', with: password - click_button 'Change your password' - end end From efeeed2a0a1798a8d5ad4bfd014f6f3f4d9f3d8c Mon Sep 17 00:00:00 2001 From: Andrey Date: Thu, 1 Oct 2015 14:06:29 +0200 Subject: [PATCH 05/27] content block height fix --- app/assets/stylesheets/generic/sidebar.scss | 46 ++++++++++++++++++++- app/views/layouts/_page.html.haml | 2 +- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/generic/sidebar.scss b/app/assets/stylesheets/generic/sidebar.scss index c5ea3aca7c..39532f2899 100644 --- a/app/assets/stylesheets/generic/sidebar.scss +++ b/app/assets/stylesheets/generic/sidebar.scss @@ -18,15 +18,20 @@ } .content-wrapper { - min-height: 100vh; + min-height: 900px; + display: table; width: 100%; padding: 20px; background: #EAEBEC; + height: 100%; + width: 100%; .container-fluid { background: #FFF; padding: $gl-padding; - min-height: 90vh; + /*min-height: 90vh;*/ + height: 100%; + min-height: 100%; &.container-blank { background: none; @@ -36,6 +41,43 @@ } } + +.content { + height: 100%; + width: 100%; + +} + +.max_height { + height: 100%; + display: table; + width: 100%; +} + +.project-show-readme { + height: 100%; + display: table-row; +} + +.wiki { + min-width: 1167px; +} + +section { + height: 100%; + display: table-row; +} + +html, body { + height: 100%; + margin: 0; +} + +.page-with-sidebar{ + min-height: 100%; + height: 100%; +} + .nav-sidebar { margin-top: 14 + $header-height; margin-bottom: 100px; diff --git a/app/views/layouts/_page.html.haml b/app/views/layouts/_page.html.haml index 2468687b56..95a6267e2e 100644 --- a/app/views/layouts/_page.html.haml +++ b/app/views/layouts/_page.html.haml @@ -25,5 +25,5 @@ = render "layouts/flash" %div{ class: container_class } .content - .clearfix + .clearfix.max_height = yield From a4383aea4ce5d610509d0019b85dabe90695deec Mon Sep 17 00:00:00 2001 From: Andrey Date: Thu, 1 Oct 2015 14:18:12 +0200 Subject: [PATCH 06/27] css aligning --- app/assets/stylesheets/generic/sidebar.scss | 1 - 1 file changed, 1 deletion(-) diff --git a/app/assets/stylesheets/generic/sidebar.scss b/app/assets/stylesheets/generic/sidebar.scss index 39532f2899..e3a5b7ad65 100644 --- a/app/assets/stylesheets/generic/sidebar.scss +++ b/app/assets/stylesheets/generic/sidebar.scss @@ -29,7 +29,6 @@ .container-fluid { background: #FFF; padding: $gl-padding; - /*min-height: 90vh;*/ height: 100%; min-height: 100%; From e3bc0e210451494ad5290755a79a510ea5c9a18b Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 1 Oct 2015 18:10:09 +0200 Subject: [PATCH 07/27] Ensure GitLab CI project exists when CI service is activated manually When I check activeated checkbox in project services for GitLab CI it cause half-working state when gitlab_ci_project is missing. This patch fixes it until we have proper behaviour implemented later Signed-off-by: Dmitriy Zaporozhets --- app/models/project_services/gitlab_ci_service.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/models/project_services/gitlab_ci_service.rb b/app/models/project_services/gitlab_ci_service.rb index 436d4cfed8..a095eaaada 100644 --- a/app/models/project_services/gitlab_ci_service.rb +++ b/app/models/project_services/gitlab_ci_service.rb @@ -22,12 +22,17 @@ class GitlabCiService < CiService include Gitlab::Application.routes.url_helpers after_save :compose_service_hook, if: :activated? + after_save :ensure_gitlab_ci_project, if: :activated? def compose_service_hook hook = service_hook || build_service_hook hook.save end + def ensure_gitlab_ci_project + project.ensure_gitlab_ci_project + end + def supported_events %w(push tag_push) end From 788a3f9b94f5500bbbe3f3c4955c6c41196a736e Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Tue, 15 Sep 2015 15:33:44 -0500 Subject: [PATCH 08/27] Add last push widget to starred projects dashboard --- app/controllers/dashboard/projects_controller.rb | 1 + app/views/dashboard/projects/starred.html.haml | 3 +++ 2 files changed, 4 insertions(+) diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index 467d0f81ac..58e9049f15 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -20,6 +20,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController @projects = current_user.starred_projects @projects = @projects.includes(:namespace, :forked_from_project, :tags) @projects = @projects.sort(@sort = params[:sort]) + @last_push = current_user.recent_push @groups = [] respond_to do |format| diff --git a/app/views/dashboard/projects/starred.html.haml b/app/views/dashboard/projects/starred.html.haml index 339362701d..f75f2e0a32 100644 --- a/app/views/dashboard/projects/starred.html.haml +++ b/app/views/dashboard/projects/starred.html.haml @@ -3,6 +3,9 @@ = render 'dashboard/projects_head' +- if @last_push + = render "events/event_last_push", event: @last_push + - if @projects.any? = render 'projects' - else From 19748ddee6ef4c794d6cc30cdf1c607088cc5bf7 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 1 Oct 2015 21:38:39 -0400 Subject: [PATCH 09/27] Update config/locales/devise.en.yml with latest version It looks like a lot of changes but it's not, they just sorted it alphabetically. --- config/locales/devise.en.yml | 103 ++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 51 deletions(-) diff --git a/config/locales/devise.en.yml b/config/locales/devise.en.yml index d8bf0878a3..bd4c3ebc69 100644 --- a/config/locales/devise.en.yml +++ b/config/locales/devise.en.yml @@ -1,61 +1,62 @@ -# Additional translations at http://github.com/plataformatec/devise/wiki/I18n +# Additional translations at https://github.com/plataformatec/devise/wiki/I18n en: + devise: + confirmations: + confirmed: "Your email address has been successfully confirmed." + send_instructions: "You will receive an email with instructions for how to confirm your email address in a few minutes." + send_paranoid_instructions: "If your email address exists in our database, you will receive an email with instructions for how to confirm your email address in a few minutes." + failure: + already_authenticated: "You are already signed in." + inactive: "Your account is not activated yet." + invalid: "Invalid %{authentication_keys} or password." + locked: "Your account is locked." + last_attempt: "You have one more attempt before your account is locked." + not_found_in_database: "Invalid %{authentication_keys} or password." + timeout: "Your session expired. Please sign in again to continue." + unauthenticated: "You need to sign in or sign up before continuing." + unconfirmed: "You have to confirm your email address before continuing." + mailer: + confirmation_instructions: + subject: "Confirmation instructions" + reset_password_instructions: + subject: "Reset password instructions" + unlock_instructions: + subject: "Unlock instructions" + password_change: + subject: "Password Changed" + omniauth_callbacks: + failure: "Could not authenticate you from %{kind} because \"%{reason}\"." + success: "Successfully authenticated from %{kind} account." + passwords: + no_token: "You can't access this page without coming from a password reset email. If you do come from a password reset email, please make sure you used the full URL provided." + send_instructions: "You will receive an email with instructions on how to reset your password in a few minutes." + send_paranoid_instructions: "If your email address exists in our database, you will receive a password recovery link at your email address in a few minutes." + updated: "Your password has been changed successfully. You are now signed in." + updated_not_active: "Your password has been changed successfully." + registrations: + destroyed: "Bye! Your account has been successfully cancelled. We hope to see you again soon." + signed_up: "Welcome! You have signed up successfully." + signed_up_but_inactive: "You have signed up successfully. However, we could not sign you in because your account is not yet activated." + signed_up_but_locked: "You have signed up successfully. However, we could not sign you in because your account is locked." + signed_up_but_unconfirmed: "A message with a confirmation link has been sent to your email address. Please follow the link to activate your account." + update_needs_confirmation: "You updated your account successfully, but we need to verify your new email address. Please check your email and follow the confirm link to confirm your new email address." + updated: "Your account has been updated successfully." + sessions: + signed_in: "Signed in successfully." + signed_out: "Signed out successfully." + already_signed_out: "Signed out successfully." + unlocks: + send_instructions: "You will receive an email with instructions for how to unlock your account in a few minutes." + send_paranoid_instructions: "If your account exists, you will receive an email with instructions for how to unlock it in a few minutes." + unlocked: "Your account has been unlocked successfully. Please sign in to continue." errors: messages: + already_confirmed: "was already confirmed, please try signing in" + confirmation_period_expired: "needs to be confirmed within %{period}, please request a new one" expired: "has expired, please request a new one" not_found: "not found" - already_confirmed: "was already confirmed, please try signing in" not_locked: "was not locked" not_saved: one: "1 error prohibited this %{resource} from being saved:" other: "%{count} errors prohibited this %{resource} from being saved:" - - devise: - failure: - already_authenticated: 'You are already signed in.' - unauthenticated: 'You need to sign in before continuing.' - unconfirmed: 'You have to confirm your account before continuing.' - locked: 'Your account is locked.' - not_found_in_database: 'Invalid email or password.' - invalid: 'Invalid email or password.' - invalid_token: 'Invalid authentication token.' - timeout: 'Your session expired, please sign in again to continue.' - inactive: 'Your account was not activated yet.' - sessions: - signed_in: '' - signed_out: '' - users_sessions: - user: - signed_in: 'Signed in successfully.' - passwords: - send_instructions: 'You will receive an email with instructions about how to reset your password in a few minutes.' - updated: 'Your password was changed successfully. You are now signed in.' - updated_not_active: 'Your password was changed successfully.' - send_paranoid_instructions: "If your email address exists in our database, you will receive a password recovery link at your email address in a few minutes." - no_token: "You can't access this page without coming from a password reset email. If you do come from a password reset email, please make sure you used the full URL provided." - confirmations: - send_instructions: 'You will receive an email with instructions about how to confirm your account in a few minutes.' - send_paranoid_instructions: 'If your email address exists in our database, you will receive an email with instructions about how to confirm your account in a few minutes.' - confirmed: 'Your account was successfully confirmed. You are now signed in.' - registrations: - signed_up: 'Welcome! You have signed up successfully.' - updated: 'You updated your account successfully.' - destroyed: 'Bye! Your account was successfully cancelled. We hope to see you again soon.' - signed_up_but_unconfirmed: 'A message with a confirmation link has been sent to your email address. Please open the link to activate your account.' - signed_up_but_inactive: 'You have signed up successfully. However, we could not sign you in because your account is not yet activated.' - signed_up_but_locked: 'You have signed up successfully. However, we could not sign you in because your account is locked.' - unlocks: - send_instructions: 'You will receive an email with instructions about how to unlock your account in a few minutes.' - unlocked: 'Your account was successfully unlocked. You are now signed in.' - send_paranoid_instructions: 'If your account exists, you will receive an email with instructions about how to unlock it in a few minutes.' - omniauth_callbacks: - success: 'Successfully authorized from %{kind} account.' - failure: 'Could not authorize you from %{kind} because "%{reason}".' - mailer: - confirmation_instructions: - subject: 'Confirmation instructions' - reset_password_instructions: - subject: 'Reset password instructions' - unlock_instructions: - subject: 'Unlock Instructions' From ad7ad8745a33581680091c5ade9377c0aae74715 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 1 Oct 2015 21:41:56 -0400 Subject: [PATCH 10/27] Add User#recently_sent_password_reset? --- app/models/user.rb | 4 ++++ spec/models/user_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index 3879f3fd38..6a1e5fd52e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -327,6 +327,10 @@ class User < ActiveRecord::Base @reset_token end + def recently_sent_password_reset? + reset_password_sent_at.present? && reset_password_sent_at >= 1.minute.ago + end + def disable_two_factor! update_attributes( two_factor_enabled: false, diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 480950859a..b45c78f38d 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -227,6 +227,26 @@ describe User do end end + describe 'recently_sent_password_reset?' do + it 'is false when reset_password_sent_at is nil' do + user = build_stubbed(:user, reset_password_sent_at: nil) + + expect(user.recently_sent_password_reset?).to eq false + end + + it 'is false when sent more than one minute ago' do + user = build_stubbed(:user, reset_password_sent_at: 5.minutes.ago) + + expect(user.recently_sent_password_reset?).to eq false + end + + it 'is true when sent less than one minute ago' do + user = build_stubbed(:user, reset_password_sent_at: Time.now) + + expect(user.recently_sent_password_reset?).to eq true + end + end + describe '#disable_two_factor!' do it 'clears all 2FA-related fields' do user = create(:user, :two_factor) From c7b43126bd7f5ef1b76a546029754ee44d68288e Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 1 Oct 2015 21:46:51 -0400 Subject: [PATCH 11/27] Add recently_reset message to Devise translations --- config/locales/devise.en.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/config/locales/devise.en.yml b/config/locales/devise.en.yml index bd4c3ebc69..22070e37f0 100644 --- a/config/locales/devise.en.yml +++ b/config/locales/devise.en.yml @@ -30,6 +30,7 @@ en: success: "Successfully authenticated from %{kind} account." passwords: no_token: "You can't access this page without coming from a password reset email. If you do come from a password reset email, please make sure you used the full URL provided." + recently_reset: "Instructions about how to reset your password have already been sent recently. Please wait a few minutes to try again." send_instructions: "You will receive an email with instructions on how to reset your password in a few minutes." send_paranoid_instructions: "If your email address exists in our database, you will receive a password recovery link at your email address in a few minutes." updated: "Your password has been changed successfully. You are now signed in." From b8ff38b1d47e4323f799b593b95821ed4a8c11f7 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 1 Oct 2015 21:47:27 -0400 Subject: [PATCH 12/27] Refactor PasswordsController to use before_actions --- app/controllers/passwords_controller.rb | 42 ++++++++++++------------- spec/features/password_reset_spec.rb | 2 +- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index a2d152addc..2025158d06 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -1,21 +1,7 @@ class PasswordsController < Devise::PasswordsController - - def create - email = resource_params[:email] - self.resource = resource_class.find_by_email(email) - - if resource && resource.ldap_user? - flash[:alert] = "Cannot reset password for LDAP user." - respond_with({}, location: after_sending_reset_password_instructions_path_for(resource_name)) and return - end - - unless can_send_reset_email? - flash[:alert] = "Instructions about how to reset your password have already been sent recently. Please wait a few minutes to try again." - respond_with({}, location: new_password_path(resource_name)) and return - end - - super - end + before_action :resource_from_email, only: [:create] + before_action :prevent_ldap_reset, only: [:create] + before_action :throttle_reset, only: [:create] def edit super @@ -37,10 +23,24 @@ class PasswordsController < Devise::PasswordsController end end - private + protected - def can_send_reset_email? - resource && (resource.reset_password_sent_at.blank? || - resource.reset_password_sent_at < 1.minute.ago) + def resource_from_email + email = resource_params[:email] + self.resource = resource_class.find_by_email(email) + end + + def prevent_ldap_reset + return unless resource && resource.ldap_user? + + redirect_to after_sending_reset_password_instructions_path_for(resource_name), + alert: "Cannot reset password for LDAP user." + end + + def throttle_reset + return unless resource && resource.recently_sent_password_reset? + + redirect_to new_password_path(resource_name), + alert: I18n.t('devise.passwords.recently_reset') end end diff --git a/spec/features/password_reset_spec.rb b/spec/features/password_reset_spec.rb index 4d512c6543..85e70b4d47 100644 --- a/spec/features/password_reset_spec.rb +++ b/spec/features/password_reset_spec.rb @@ -29,7 +29,7 @@ feature 'Password reset', feature: true do visit root_path forgot_password(user) - expect(page).to have_content("Instructions about how to reset your password have already been sent recently. Please wait a few minutes to try again.") + expect(page).to have_content(I18n.t('devise.passwords.recently_reset')) expect(current_path).to eq new_user_password_path end end From f036d4095e5700a1663ce3429b0342fa813c3c7a Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 1 Oct 2015 23:46:43 -0400 Subject: [PATCH 13/27] Fix spec broken by updated Devise translations --- spec/features/login_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index cef432e512..922c76285d 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -95,7 +95,7 @@ feature 'Login', feature: true do user = create(:user, password: 'not-the-default') login_with(user) - expect(page).to have_content('Invalid email or password.') + expect(page).to have_content('Invalid login or password.') end end end From d40dd5cfe331c5e465b77c8eecae9697c873a67a Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 2 Oct 2015 00:14:47 -0400 Subject: [PATCH 14/27] Conform to spec guidelines that only exist in my head [ci skip] --- spec/models/user_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b45c78f38d..5e6918a5ac 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -227,7 +227,7 @@ describe User do end end - describe 'recently_sent_password_reset?' do + describe '#recently_sent_password_reset?' do it 'is false when reset_password_sent_at is nil' do user = build_stubbed(:user, reset_password_sent_at: nil) From 5de0b078442da2adc2b0673e3286c7d1a7cb2501 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 2 Oct 2015 10:08:16 +0200 Subject: [PATCH 15/27] Prevent creating 2 Ci::Project entities when enable CI Signed-off-by: Dmitriy Zaporozhets --- app/models/project.rb | 6 +----- app/services/git_push_service.rb | 2 +- features/steps/project/commits/commits.rb | 2 +- features/steps/shared/project.rb | 2 +- spec/models/project_spec.rb | 3 +-- 5 files changed, 5 insertions(+), 10 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 8527fa2980..fa7690d8fd 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -751,13 +751,9 @@ class Project < ActiveRecord::Base gitlab_ci_project || create_gitlab_ci_project end - def enable_ci(user) - # Enable service + def enable_ci service = gitlab_ci_service || create_gitlab_ci_service service.active = true service.save - - # Create Ci::Project - Ci::CreateProjectService.new.execute(user, self) end end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 8193b6e192..f9a8265d2d 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -58,7 +58,7 @@ class GitPushService # If CI was disabled but .gitlab-ci.yml file was pushed # we enable CI automatically if !project.gitlab_ci? && gitlab_ci_yaml?(newrev) - project.enable_ci(user) + project.enable_ci end EventCreateService.new.push(project, user, @push_data) diff --git a/features/steps/project/commits/commits.rb b/features/steps/project/commits/commits.rb index 47f58091b9..5ebc3a4976 100644 --- a/features/steps/project/commits/commits.rb +++ b/features/steps/project/commits/commits.rb @@ -103,7 +103,7 @@ class Spinach::Features::ProjectCommits < Spinach::FeatureSteps end step 'commit has ci status' do - @project.enable_ci(@user) + @project.enable_ci create :ci_commit, gl_project: @project, sha: sample_commit.id end diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index fc51cec150..5744e455eb 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -199,7 +199,7 @@ module SharedProject step 'project "Shop" has CI enabled' do project = Project.find_by(name: "Shop") - project.enable_ci(@user) + project.enable_ci end step 'project "Shop" has CI build' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index fe7bb2cc13..1c3f5374a2 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -417,9 +417,8 @@ describe Project do describe :enable_ci do let(:project) { create :project } - let(:user) { create :user } - before { project.enable_ci(user) } + before { project.enable_ci } it { expect(project.gitlab_ci?).to be_truthy } it { expect(project.gitlab_ci_project).to be_a(Ci::Project) } From 37e9e71ea1162fbae13bdc9c41684bdd4ad03b1e Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 2 Oct 2015 10:26:56 +0200 Subject: [PATCH 16/27] Remove unnecessary fork ci logic Signed-off-by: Dmitriy Zaporozhets --- .../project_services/gitlab_ci_service.rb | 15 -------- app/services/ci/create_project_service.rb | 30 --------------- app/services/projects/fork_service.rb | 8 +++- .../ci/create_project_service_spec.rb | 37 ------------------- spec/services/projects/fork_service_spec.rb | 2 +- 5 files changed, 8 insertions(+), 84 deletions(-) delete mode 100644 app/services/ci/create_project_service.rb delete mode 100644 spec/services/ci/create_project_service_spec.rb diff --git a/app/models/project_services/gitlab_ci_service.rb b/app/models/project_services/gitlab_ci_service.rb index a095eaaada..6d2cf79b69 100644 --- a/app/models/project_services/gitlab_ci_service.rb +++ b/app/models/project_services/gitlab_ci_service.rb @@ -72,21 +72,6 @@ class GitlabCiService < CiService :error end - def fork_registration(new_project, current_user) - params = OpenStruct.new({ - id: new_project.id, - default_branch: new_project.default_branch - }) - - ci_project = Ci::Project.find_by!(gitlab_id: project.id) - - Ci::CreateProjectService.new.execute( - current_user, - params, - ci_project - ) - end - def commit_coverage(sha, ref) get_ci_commit(sha, ref).coverage rescue ActiveRecord::RecordNotFound diff --git a/app/services/ci/create_project_service.rb b/app/services/ci/create_project_service.rb deleted file mode 100644 index f42babd238..0000000000 --- a/app/services/ci/create_project_service.rb +++ /dev/null @@ -1,30 +0,0 @@ -module Ci - class CreateProjectService - include Gitlab::Application.routes.url_helpers - - def execute(current_user, params, forked_project = nil) - @project = Ci::Project.parse(params) - - Ci::Project.transaction do - @project.save! - - gl_project = ::Project.find(@project.gitlab_id) - gl_project.build_missing_services - gl_project.gitlab_ci_service.update_attributes(active: true) - end - - if forked_project - # Copy settings - settings = forked_project.attributes.select do |attr_name, value| - ["public", "shared_runners_enabled", "allow_git_fetch"].include? attr_name - end - - @project.update(settings) - end - - Ci::EventService.new.create_project(current_user, @project) - - @project - end - end -end diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb index 2e995d6fd5..46374a3909 100644 --- a/app/services/projects/fork_service.rb +++ b/app/services/projects/fork_service.rb @@ -18,7 +18,13 @@ module Projects if new_project.persisted? if @project.gitlab_ci? - @project.gitlab_ci_service.fork_registration(new_project, @current_user) + new_project.enable_ci + + settings = @project.gitlab_ci_project.attributes.select do |attr_name, value| + ["public", "shared_runners_enabled", "allow_git_fetch"].include? attr_name + end + + new_project.gitlab_ci_project.update(settings) end end diff --git a/spec/services/ci/create_project_service_spec.rb b/spec/services/ci/create_project_service_spec.rb deleted file mode 100644 index 2de7b0deca..0000000000 --- a/spec/services/ci/create_project_service_spec.rb +++ /dev/null @@ -1,37 +0,0 @@ -require 'spec_helper' - -describe Ci::CreateProjectService do - let(:service) { Ci::CreateProjectService.new } - let(:current_user) { double.as_null_object } - let(:project) { FactoryGirl.create :project } - - describe :execute do - context 'valid params' do - subject { service.execute(current_user, project) } - - it { is_expected.to be_kind_of(Ci::Project) } - it { is_expected.to be_persisted } - end - - context 'without project dump' do - it 'should raise exception' do - expect { service.execute(current_user, '', '') }. - to raise_error(NoMethodError) - end - end - - context "forking" do - let(:ci_origin_project) do - FactoryGirl.create(:ci_project, shared_runners_enabled: true, public: true, allow_git_fetch: true) - end - - subject { service.execute(current_user, project, ci_origin_project) } - - it "uses project as a template for settings and jobs" do - expect(subject.shared_runners_enabled).to be_truthy - expect(subject.public).to be_truthy - expect(subject.allow_git_fetch).to be_truthy - end - end - end -end diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 18ab333c1d..a850aa7cbd 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -48,7 +48,7 @@ describe Projects::ForkService do @from_project.build_missing_services @from_project.gitlab_ci_service.update_attributes(active: true) - expect_any_instance_of(Ci::CreateProjectService).to receive(:execute) + expect_any_instance_of(Project).to receive(:enable_ci) fork_project(@from_project, @to_user) end From 3515cb9b2de4b6c1018fec35b0513eb7dc33dc66 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 2 Oct 2015 11:02:05 +0200 Subject: [PATCH 17/27] Fix tests Signed-off-by: Dmitriy Zaporozhets --- .../gitlab_ci_service_spec.rb | 21 ------------------- spec/services/projects/fork_service_spec.rb | 12 ++++------- 2 files changed, 4 insertions(+), 29 deletions(-) diff --git a/spec/models/project_services/gitlab_ci_service_spec.rb b/spec/models/project_services/gitlab_ci_service_spec.rb index 8cdd551a0c..989cfe0916 100644 --- a/spec/models/project_services/gitlab_ci_service_spec.rb +++ b/spec/models/project_services/gitlab_ci_service_spec.rb @@ -56,25 +56,4 @@ describe GitlabCiService do end end end - - describe "Fork registration" do - before do - @old_project = create(:ci_project).gl_project - @project = create(:empty_project) - @user = create(:user) - - @service = GitlabCiService.new - allow(@service).to receive_messages( - service_hook: true, - project_url: 'http://ci.gitlab.org/projects/2', - token: 'verySecret', - project: @old_project - ) - end - - it "creates fork on CI" do - expect_any_instance_of(Ci::CreateProjectService).to receive(:execute) - @service.fork_registration(@project, @user) - end - end end diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index a850aa7cbd..65a8c81204 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -43,14 +43,10 @@ describe Projects::ForkService do end context 'GitLab CI is enabled' do - it "calls fork registrator for CI" do - create(:ci_project, gl_project: @from_project) - @from_project.build_missing_services - @from_project.gitlab_ci_service.update_attributes(active: true) - - expect_any_instance_of(Project).to receive(:enable_ci) - - fork_project(@from_project, @to_user) + it "fork and enable CI for fork" do + @from_project.enable_ci + @to_project = fork_project(@from_project, @to_user) + expect(@to_project.gitlab_ci?).to be_truthy end end end From fd86b66914c1fff35d38ed0715d64f7e9dfa2757 Mon Sep 17 00:00:00 2001 From: Andrey Date: Fri, 2 Oct 2015 11:24:12 +0200 Subject: [PATCH 18/27] CSS markup fixed Everything is fixed according DZ comments. Added a bit sexy transition for our project buttons >< --- app/assets/stylesheets/base/layout.scss | 6 ++++ app/assets/stylesheets/generic/buttons.scss | 4 +++ app/assets/stylesheets/generic/sidebar.scss | 40 ++------------------- app/assets/stylesheets/pages/projects.scss | 16 +++++++++ app/views/layouts/_page.html.haml | 2 +- 5 files changed, 30 insertions(+), 38 deletions(-) diff --git a/app/assets/stylesheets/base/layout.scss b/app/assets/stylesheets/base/layout.scss index b91c15d891..ced3769af0 100644 --- a/app/assets/stylesheets/base/layout.scss +++ b/app/assets/stylesheets/base/layout.scss @@ -1,15 +1,21 @@ html { overflow-y: scroll; + height: 100%; + margin: 0; &.touch .tooltip { display: none !important; } body { padding-top: $header-height; + height: 100%; + margin: 0; } } .container { padding-top: 0; + height: 100%; + width: 100%; z-index: 5; } diff --git a/app/assets/stylesheets/generic/buttons.scss b/app/assets/stylesheets/generic/buttons.scss index cf76f538e0..a5fe589044 100644 --- a/app/assets/stylesheets/generic/buttons.scss +++ b/app/assets/stylesheets/generic/buttons.scss @@ -94,6 +94,7 @@ body { @mixin btn-info { @include border-radius(2px); + @include transition (all 0.2s ease 0s); border-width: 1px; border-style: solid; @@ -116,6 +117,7 @@ body { &:active { @include box-shadow(inset 0 0 4px rgba(0, 0, 0, 0.12)); + border-width: 1px; border-style: solid; } @@ -123,6 +125,7 @@ body { @mixin btn-middle { @include border-radius(2px); + @include transition (all 0.2s ease 0s); border-width: 1px; border-style: solid; @@ -145,6 +148,7 @@ body { &:active { @include box-shadow(inset 0 0 4px rgba(0, 0, 0, 0.12)); + border-width: 1px; border-style: solid; } diff --git a/app/assets/stylesheets/generic/sidebar.scss b/app/assets/stylesheets/generic/sidebar.scss index e3a5b7ad65..d30fc6e189 100644 --- a/app/assets/stylesheets/generic/sidebar.scss +++ b/app/assets/stylesheets/generic/sidebar.scss @@ -1,4 +1,7 @@ .page-with-sidebar { + min-height: 100%; + height: 100%; + .sidebar-wrapper { position: fixed; top: 0; @@ -40,43 +43,6 @@ } } - -.content { - height: 100%; - width: 100%; - -} - -.max_height { - height: 100%; - display: table; - width: 100%; -} - -.project-show-readme { - height: 100%; - display: table-row; -} - -.wiki { - min-width: 1167px; -} - -section { - height: 100%; - display: table-row; -} - -html, body { - height: 100%; - margin: 0; -} - -.page-with-sidebar{ - min-height: 100%; - height: 100%; -} - .nav-sidebar { margin-top: 14 + $header-height; margin-bottom: 100px; diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index a5940543a9..c1505b9a62 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -505,3 +505,19 @@ pre.light-well { display: inline-block; } +.content { + height: 100%; + width: 100%; + +} + +.max-height { + height: 100%; + display: table; + width: 100%; +} + +section { + height: 100%; + display: table-row; +} \ No newline at end of file diff --git a/app/views/layouts/_page.html.haml b/app/views/layouts/_page.html.haml index 95a6267e2e..1f4ade81ed 100644 --- a/app/views/layouts/_page.html.haml +++ b/app/views/layouts/_page.html.haml @@ -25,5 +25,5 @@ = render "layouts/flash" %div{ class: container_class } .content - .clearfix.max_height + .clearfix.max-height = yield From 2fa89a3dc6a7580969203e43808048b79f172c0c Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 2 Oct 2015 11:29:46 +0200 Subject: [PATCH 19/27] Added benchmark-ips to the Gemfile This allows me to use this Gem for benchmarking without having to add/remove it every time. --- Gemfile | 2 ++ Gemfile.lock | 2 ++ 2 files changed, 4 insertions(+) diff --git a/Gemfile b/Gemfile index f1674c560e..4938cbf8b8 100644 --- a/Gemfile +++ b/Gemfile @@ -270,6 +270,8 @@ group :development, :test do gem 'rubocop', '~> 0.28.0', require: false gem 'coveralls', '~> 0.8.2', require: false gem 'simplecov', '~> 0.10.0', require: false + + gem 'benchmark-ips', require: false end group :test do diff --git a/Gemfile.lock b/Gemfile.lock index 6472221bc5..1dd56cd9c8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -66,6 +66,7 @@ GEM ice_nine (~> 0.11.0) thread_safe (~> 0.3, >= 0.3.1) bcrypt (3.1.10) + benchmark-ips (2.3.0) better_errors (1.0.1) coderay (>= 1.0.0) erubis (>= 2.6.6) @@ -795,6 +796,7 @@ DEPENDENCIES asciidoctor (~> 1.5.2) attr_encrypted (~> 1.3.4) awesome_print (~> 1.2.0) + benchmark-ips better_errors (~> 1.0.1) binding_of_caller (~> 0.7.2) bootstrap-sass (~> 3.0) From 731b860976772111ca673ba25aeadd3634adbe7d Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Thu, 1 Oct 2015 17:33:05 +0300 Subject: [PATCH 20/27] Hide password in the service settings form --- CHANGELOG | 1 + app/controllers/projects/services_controller.rb | 4 +++- app/views/shared/_field.html.haml | 7 +++++-- features/project/service.feature | 1 + features/steps/project/services.rb | 4 ++++ 5 files changed, 14 insertions(+), 3 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index a8b43dd460..8d95dd79af 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -26,6 +26,7 @@ v 8.1.0 (unreleased) - Show additions/deletions stats on merge request diff - Remove footer text in emails (Zeger-Jan van de Weg) - Ensure code blocks are properly highlighted after a note is updated + - Hide password in the service settings form v 8.0.3 - Fix URL shown in Slack notifications diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb index 3a22ed832a..3047ee8a1f 100644 --- a/app/controllers/projects/services_controller.rb +++ b/app/controllers/projects/services_controller.rb @@ -58,6 +58,8 @@ class Projects::ServicesController < Projects::ApplicationController end def service_params - params.require(:service).permit(ALLOWED_PARAMS) + service_params = params.require(:service).permit(ALLOWED_PARAMS) + service_params.delete("password") if service_params["password"].blank? + service_params end end diff --git a/app/views/shared/_field.html.haml b/app/views/shared/_field.html.haml index 45ec49280d..8d6e16f74c 100644 --- a/app/views/shared/_field.html.haml +++ b/app/views/shared/_field.html.haml @@ -8,7 +8,10 @@ - help = field[:help] .form-group - = form.label name, title, class: "control-label" + - if type == "password" && value.present? + = form.label name, "Change #{title}", class: "control-label" + - else + = form.label name, title, class: "control-label" .col-sm-10 - if type == 'text' = form.text_field name, class: "form-control", placeholder: placeholder @@ -19,6 +22,6 @@ - elsif type == 'select' = form.select name, options_for_select(choices, value ? value : default_choice), {}, { class: "form-control" } - elsif type == 'password' - = form.password_field name, value: value, class: 'form-control' + = form.password_field name, autocomplete: "new-password", class: 'form-control' - if help %span.help-block= help diff --git a/features/project/service.feature b/features/project/service.feature index fdff640ec8..5014b52b9f 100644 --- a/features/project/service.feature +++ b/features/project/service.feature @@ -72,6 +72,7 @@ Feature: Project Services And I click Atlassian Bamboo CI service link And I fill Atlassian Bamboo CI settings Then I should see Atlassian Bamboo CI service settings saved + And I should see empty field Change Password Scenario: Activate jetBrains TeamCity CI service When I visit project "Shop" services page diff --git a/features/steps/project/services.rb b/features/steps/project/services.rb index d3b462bfd3..1c700df0c6 100644 --- a/features/steps/project/services.rb +++ b/features/steps/project/services.rb @@ -202,6 +202,10 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps expect(find_field('Username').value).to eq 'user' end + step 'I should see empty field Change Password' do + expect(find_field('Change Password').value).to be_nil + end + step 'I click JetBrains TeamCity CI service link' do click_link 'JetBrains TeamCity CI' end From 5ebcf21b81eaf3b67d1f7c675ec54be19041a379 Mon Sep 17 00:00:00 2001 From: Andrey Date: Fri, 2 Oct 2015 12:01:46 +0200 Subject: [PATCH 21/27] section and .content now in layout.cssc --- app/assets/stylesheets/base/layout.scss | 10 ++++++++++ app/assets/stylesheets/pages/projects.scss | 11 ----------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/app/assets/stylesheets/base/layout.scss b/app/assets/stylesheets/base/layout.scss index ced3769af0..c6301ab6d3 100644 --- a/app/assets/stylesheets/base/layout.scss +++ b/app/assets/stylesheets/base/layout.scss @@ -19,6 +19,16 @@ html { z-index: 5; } +.content { + height: 100%; + width: 100%; +} + +.content section { + height: 100%; + display: table-row; +} + .container .content { margin: 0 0; } diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index c1505b9a62..ddcf65d38f 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -505,19 +505,8 @@ pre.light-well { display: inline-block; } -.content { - height: 100%; - width: 100%; - -} - .max-height { height: 100%; display: table; width: 100%; -} - -section { - height: 100%; - display: table-row; } \ No newline at end of file From a4292066f0b8dc7f0ff69b3f7a6a11424a42ad01 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 2 Oct 2015 12:44:20 +0200 Subject: [PATCH 22/27] Back and forth permission on builds/ --- doc/migrate_ci_to_ce/README.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/doc/migrate_ci_to_ce/README.md b/doc/migrate_ci_to_ce/README.md index 46ce0fe98c..1cb1bc2e76 100644 --- a/doc/migrate_ci_to_ce/README.md +++ b/doc/migrate_ci_to_ce/README.md @@ -182,6 +182,7 @@ will need this file later. ``` # On your CI server: # Omnibus +sudo chown gitlab-ci:gitlab-ci /var/opt/gitlab/gitlab-ci/builds sudo gitlab-ci-rake backup:create # Source @@ -228,6 +229,7 @@ be no CI data yet because you turned CI on the GitLab server off earlier. ``` # On your GitLab server: # Omnibus +sudo chown git:git /var/opt/gitlab/gitlab-ci/builds sudo gitlab-rake ci:migrate # Source @@ -342,7 +344,12 @@ The fix for this is to update to Omnibus 7.14 first and then update it to 8.0. #### Permission denied when accessing /var/opt/gitlab/gitlab-ci/builds To fix that issue you have to change builds/ folder permission before doing final backup: ``` -chown -R gitlab-ci:gitlab-ci /var/opt/gitlab/gitlab-ci/builds +sudo chown -R gitlab-ci:gitlab-ci /var/opt/gitlab/gitlab-ci/builds +``` + +Then before executing `ci:migrate` you need to fix builds folder permission: +``` +sudo chown git:git /var/opt/gitlab/gitlab-ci/builds ``` #### Problems when importing CI database to GitLab From 97e6c9b42ca65be14f64f1528821922b2b0bd04a Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Fri, 2 Oct 2015 15:10:33 +0300 Subject: [PATCH 23/27] Wrong access level badge on MR comments --- CHANGELOG | 1 + app/models/project_team.rb | 4 ++++ app/views/projects/notes/_note.html.haml | 6 +++--- spec/models/project_team_spec.rb | 12 ++++++++++++ 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index a8b43dd460..d0bc80daaf 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -26,6 +26,7 @@ v 8.1.0 (unreleased) - Show additions/deletions stats on merge request diff - Remove footer text in emails (Zeger-Jan van de Weg) - Ensure code blocks are properly highlighted after a note is updated + - Fix wrong access level badge on MR comments v 8.0.3 - Fix URL shown in Slack notifications diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 56e49af232..f602a96536 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -135,6 +135,10 @@ class ProjectTeam !!find_member(user_id) end + def human_max_access(user_id) + Gitlab::Access.options.key max_member_access(user_id) + end + def max_member_access(user_id) access = [] access << project.project_members.find_by(user_id: user_id).try(:access_field) diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index cf5d5d6d8b..1638ad6891 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -14,10 +14,10 @@ = icon('trash-o') - unless note.system - - member = note.project.team.find_member(note.author.id) - - if member + - access = note.project.team.human_max_access(note.author.id) + - if access %span.note-role.label - = member.human_access + = access = link_to_member(note.project, note.author, avatar: false) diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index cc1138490a..26e8fdae47 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -66,4 +66,16 @@ describe ProjectTeam do it { expect(project.team.member?(guest)).to be_truthy } end end + + describe "#human_max_access" do + it "return master role" do + user = create :user + group = create :group + group.add_users([user.id], GroupMember::MASTER) + project = create(:project, namespace: group) + project.team << [user, :guest] + + expect(project.team.human_max_access(user.id)).to eq("Master") + end + end end From acdb5f34cffc124ac9f727ff434f375b0bbe9971 Mon Sep 17 00:00:00 2001 From: Andrey Date: Fri, 2 Oct 2015 16:38:37 +0200 Subject: [PATCH 24/27] max height to layout.scss --- app/assets/stylesheets/base/layout.scss | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/assets/stylesheets/base/layout.scss b/app/assets/stylesheets/base/layout.scss index c6301ab6d3..f0569a5e67 100644 --- a/app/assets/stylesheets/base/layout.scss +++ b/app/assets/stylesheets/base/layout.scss @@ -40,3 +40,9 @@ html { .container-limited { max-width: $fixed-layout-width; } + +.max-height { + height: 100%; + display: table; + width: 100%; +} \ No newline at end of file From 75c03530f8c85924d0b7dfc020ebe65ec0488dba Mon Sep 17 00:00:00 2001 From: Andrey Date: Fri, 2 Oct 2015 17:08:12 +0200 Subject: [PATCH 25/27] removed max-heght from project.scss --- app/assets/stylesheets/pages/projects.scss | 6 ------ 1 file changed, 6 deletions(-) diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index ddcf65d38f..acf0744012 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -503,10 +503,4 @@ pre.light-well { .inline-form { display: inline-block; -} - -.max-height { - height: 100%; - display: table; - width: 100%; } \ No newline at end of file From dbc85bfa01d9ee37e466bd537a20f1cfde89be9f Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 1 Oct 2015 22:11:10 -0700 Subject: [PATCH 26/27] Fix bug where transferring a project would result in stale commit links Transferring a project to another namespace updates the project's updated_at field, but since the cache key did not depend on the object, the page fragments were not invalidated. This resulted in stale links to the commits. Changing the cache key to use the object pathname solves this issue. Closes gitlab-org/omnibus-gitlab#843 --- CHANGELOG | 1 + app/views/projects/commits/_commit.html.haml | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index f01da3fe7d..ec23d0f117 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.1.0 (unreleased) + - Fix bug where transferring a project would result in stale commit links (Stan Hu) - Include full path of source and target branch names in New Merge Request page (Stan Hu) - Fix Message-ID header to be RFC 2111-compliant to prevent e-mails being dropped (Stan Hu) - Add user preference to view activities as default dashboard (Stan Hu) diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index efad4cb147..cddd5aa3a8 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -5,7 +5,7 @@ - note_count = notes.user.count - ci_commit = project.ci_commit(commit.sha) -- cache_key = [project.id, commit.id, note_count] +- cache_key = [project.path_with_namespace, commit.id, note_count] - cache_key.push(ci_commit.status) if ci_commit = cache(cache_key) do From 3fbcc51102ad12948efe7d0fd1d69c1150aba416 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 2 Oct 2015 13:03:51 -0700 Subject: [PATCH 27/27] Update README cache key to use full project namespace --- app/helpers/projects_helper.rb | 2 +- spec/helpers/projects_helper_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 7b4747ce3d..a0220af4c3 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -296,7 +296,7 @@ module ProjectsHelper def readme_cache_key sha = @project.commit.try(:sha) || 'nil' - [@project.id, sha, "readme"].join('-') + [@project.path_with_namespace, sha, "readme"].join('-') end def round_commit_count(project) diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 99abb95d90..53e56ebff4 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -61,13 +61,13 @@ describe ProjectsHelper do end it "returns a valid cach key" do - expect(helper.send(:readme_cache_key)).to eq("#{project.id}-#{project.commit.id}-readme") + expect(helper.send(:readme_cache_key)).to eq("#{project.path_with_namespace}-#{project.commit.id}-readme") end it "returns a valid cache key if HEAD does not exist" do allow(project).to receive(:commit) { nil } - expect(helper.send(:readme_cache_key)).to eq("#{project.id}-nil-readme") + expect(helper.send(:readme_cache_key)).to eq("#{project.path_with_namespace}-nil-readme") end end end