From 53866711d4c4f728f86d061984437bdd65f15bed Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 17 May 2016 17:12:23 +0100 Subject: [PATCH 01/42] Show loading indicator for autocomplete Closes #15435 --- .../javascripts/gfm_auto_complete.js.coffee | 78 +++++++++++++++++-- 1 file changed, 73 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/gfm_auto_complete.js.coffee b/app/assets/javascripts/gfm_auto_complete.js.coffee index 41dba34210..b13a431a52 100644 --- a/app/assets/javascripts/gfm_auto_complete.js.coffee +++ b/app/assets/javascripts/gfm_auto_complete.js.coffee @@ -22,6 +22,24 @@ GitLab.GfmAutoComplete = Milestones: template: '
  • ${title}
  • ' + Loading: + template: '
  • Loading...
  • ' + + DefaultOptions: + sorter: (query, items, searchKey) -> + return items if items[0].name? and items[0].name is 'loading' + + $.fn.atwho.default.callbacks.sorter(query, items, searchKey) + filter: (query, data, searchKey) -> + return data if data[0] is 'loading' + + $.fn.atwho.default.callbacks.filter(query, data, searchKey) + beforeInsert: (value) -> + if value.indexOf('undefined') + @at + else + value + # Add GFM auto-completion to all input fields, that accept GFM input. setup: (wrap) -> @input = $('.js-gfm-input') @@ -53,18 +71,37 @@ GitLab.GfmAutoComplete = # Emoji @input.atwho at: ':' - displayTpl: @Emoji.template + displayTpl: (value) => + if value.path? + @Emoji.template + else + @Loading.template insertTpl: ':${name}:' + data: ['loading'] + callbacks: + sorter: @DefaultOptions.sorter + filter: @DefaultOptions.filter + beforeInsert: @DefaultOptions.beforeInsert # Team Members @input.atwho at: '@' - displayTpl: @Members.template + displayTpl: (value) => + if value.username? + @Members.template + else + @Loading.template insertTpl: '${atwho-at}${username}' searchKey: 'search' + data: ['loading'] callbacks: + sorter: @DefaultOptions.sorter + filter: @DefaultOptions.filter + beforeInsert: @DefaultOptions.beforeInsert beforeSave: (members) -> $.map members, (m) -> + return m if not m.username? + title = m.name title += " (#{m.count})" if m.count @@ -76,11 +113,21 @@ GitLab.GfmAutoComplete = at: '#' alias: 'issues' searchKey: 'search' - displayTpl: @Issues.template + displayTpl: (value) => + if value.title? + @Issues.template + else + @Loading.template + data: ['loading'] insertTpl: '${atwho-at}${id}' callbacks: + sorter: @DefaultOptions.sorter + filter: @DefaultOptions.filter + beforeInsert: @DefaultOptions.beforeInsert beforeSave: (issues) -> $.map issues, (i) -> + return i if not i.title? + id: i.iid title: sanitize(i.title) search: "#{i.iid} #{i.title}" @@ -89,11 +136,18 @@ GitLab.GfmAutoComplete = at: '%' alias: 'milestones' searchKey: 'search' - displayTpl: @Milestones.template + displayTpl: (value) => + if value.title? + @Milestones.template + else + @Loading.template insertTpl: '${atwho-at}"${title}"' + data: ['loading'] callbacks: beforeSave: (milestones) -> $.map milestones, (m) -> + return m if not m.title? + id: m.iid title: sanitize(m.title) search: "#{m.title}" @@ -102,11 +156,21 @@ GitLab.GfmAutoComplete = at: '!' alias: 'mergerequests' searchKey: 'search' - displayTpl: @Issues.template + displayTpl: (value) => + if value.title? + @Issues.template + else + @Loading.template + data: ['loading'] insertTpl: '${atwho-at}${id}' callbacks: + sorter: @DefaultOptions.sorter + filter: @DefaultOptions.filter + beforeInsert: @DefaultOptions.beforeInsert beforeSave: (merges) -> $.map merges, (m) -> + return m if not m.title? + id: m.iid title: sanitize(m.title) search: "#{m.iid} #{m.title}" @@ -128,3 +192,7 @@ GitLab.GfmAutoComplete = @input.atwho 'load', 'mergerequests', data.mergerequests # load emojis @input.atwho 'load', ':', data.emojis + + # This trigger at.js again + # otherwise we would be stuck with loading until the user types + $(':focus').trigger('keyup') From 57a3f2845653da1926ea38c061db0b9b08b2902a Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 3 Jun 2016 11:48:11 +0100 Subject: [PATCH 02/42] Added minimum password length to password field Closes #17765 --- app/controllers/sessions_controller.rb | 1 + app/views/devise/shared/_signup_box.html.haml | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index d68c2a708e..c1b940bf9e 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -14,6 +14,7 @@ class SessionsController < Devise::SessionsController before_action :load_recaptcha def new + set_minimum_password_length if Gitlab.config.ldap.enabled @ldap_servers = Gitlab::LDAP::Config.servers else diff --git a/app/views/devise/shared/_signup_box.html.haml b/app/views/devise/shared/_signup_box.html.haml index 510215bb8c..905a8dbcd8 100644 --- a/app/views/devise/shared/_signup_box.html.haml +++ b/app/views/devise/shared/_signup_box.html.haml @@ -16,7 +16,7 @@ %div = f.email_field :email, class: "form-control middle", placeholder: "Email", required: true .form-group.append-bottom-20#password-strength - = f.password_field :password, class: "form-control bottom", placeholder: "Password", required: true + = f.password_field :password, class: "form-control bottom", placeholder: "Password - minimum length #{@minimum_password_length} characters", required: true, pattern: ".{#{@minimum_password_length},}", title: "Minimum length is #{@minimum_password_length} characters" %div - if current_application_settings.recaptcha_enabled = recaptcha_tags From 1faed033ce12fb5a7190a3393e43cbea0154cb8a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 4 Jun 2016 20:43:18 +0200 Subject: [PATCH 03/42] Disable Rails/UniqBeforePluck rubocop cop --- .rubocop.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.rubocop.yml b/.rubocop.yml index 3593ae29f2..bbe4e1ece3 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1088,6 +1088,9 @@ Rails/TimeZone: Rails/Validation: Enabled: false +Rails/UniqBeforePluck: + Enabled: false + ##################### RSpec ################################## # Check that instances are not being stubbed globally. From 7038440e342a521807b1e5ffb6d47d4c0b13048d Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Mon, 6 Jun 2016 18:47:49 -0500 Subject: [PATCH 04/42] Adjust the SAML control flow to allow LDAP identities to be added to an existing SAML user. --- lib/gitlab/o_auth/user.rb | 2 +- lib/gitlab/saml/user.rb | 28 ++++++++++++++++++++++++++-- spec/lib/gitlab/saml/user_spec.rb | 19 +++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 356e96fcba..268ee11502 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -96,7 +96,7 @@ module Gitlab # Look for a corresponding person with same uid in any of the configured LDAP providers Gitlab::LDAP::Config.providers.each do |provider| adapter = Gitlab::LDAP::Adapter.new(provider) - @ldap_person = Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter) + @ldap_person = Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter) break if @ldap_person end @ldap_person diff --git a/lib/gitlab/saml/user.rb b/lib/gitlab/saml/user.rb index dba4bbfc89..6f7d4825ae 100644 --- a/lib/gitlab/saml/user.rb +++ b/lib/gitlab/saml/user.rb @@ -12,12 +12,12 @@ module Gitlab end def gl_user - @user ||= find_by_uid_and_provider - if auto_link_ldap_user? @user ||= find_or_create_ldap_user end + @user ||= find_by_uid_and_provider + if auto_link_saml_user? @user ||= find_by_email end @@ -62,6 +62,30 @@ module Gitlab !Gitlab::Saml::Config.external_groups.nil? end + def find_or_create_ldap_user + return unless ldap_person + + # If a corresponding person exists with same uid in a LDAP server, + # check if the user already has a GitLab account + user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider) + if user + # Case when a LDAP user already exists in Gitlab. Add the SAML identity to existing account. + user.identities.build(extern_uid: auth_hash.uid, provider: auth_hash.provider) + else + # No account found using LDAP in Gitlab yet: check if there is a SAML account with + # the passed uid and provider + user = find_by_uid_and_provider + if user.nil? + # No SAML account found, build a new user. + user = build_new_user + end + # Correct account is present, add the LDAP Identity to the user. + user.identities.new(provider: ldap_person.provider, extern_uid: ldap_person.dn) + end + + user + end + def auth_hash=(auth_hash) @auth_hash = Gitlab::Saml::AuthHash.new(auth_hash) end diff --git a/spec/lib/gitlab/saml/user_spec.rb b/spec/lib/gitlab/saml/user_spec.rb index c2a51d9249..f0a17244ff 100644 --- a/spec/lib/gitlab/saml/user_spec.rb +++ b/spec/lib/gitlab/saml/user_spec.rb @@ -145,6 +145,7 @@ describe Gitlab::Saml::User, lib: true do allow(ldap_user).to receive(:email) { %w(john@mail.com john2@example.com) } allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' } allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user) + allow(Gitlab::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user) end context 'and no account for the LDAP user' do @@ -177,6 +178,24 @@ describe Gitlab::Saml::User, lib: true do ]) end end + + context 'user has SAML user, and wants to add their LDAP identity' do + it 'adds the LDAP identity to the existing SAML user' do + create(:omniauth_user, email: 'john@mail.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'saml', username: 'john') + local_hash = OmniAuth::AuthHash.new(uid: 'uid=user1,ou=People,dc=example', provider: provider, info: info_hash, extra: { raw_info: OneLogin::RubySaml::Attributes.new({ 'groups' => %w(Developers Freelancers Designers) }) }) + local_saml_user = described_class.new(local_hash) + + local_saml_user.save + local_gl_user = local_saml_user.gl_user + expect(local_gl_user).to be_valid + expect(local_gl_user.identities.length).to eql 2 + identities_as_hash = local_gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } } + expect(identities_as_hash).to match_array([ { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' }, + { provider: 'saml', extern_uid: 'uid=user1,ou=People,dc=example' } + ]) + end + + end end end end From c593154cb4f0215851fbbae1dde753dacbaa6713 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Tue, 7 Jun 2016 11:19:19 -0500 Subject: [PATCH 05/42] Moved `find_or_create_ldap_user` method to parent class and added logging. --- lib/gitlab/o_auth/user.rb | 18 ++++++++++++------ lib/gitlab/saml/user.rb | 24 ------------------------ spec/lib/gitlab/saml/user_spec.rb | 2 +- 3 files changed, 13 insertions(+), 31 deletions(-) diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 268ee11502..5e52979093 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -69,13 +69,19 @@ module Gitlab return unless ldap_person # If a corresponding person exists with same uid in a LDAP server, - # set up a Gitlab user with dual LDAP and Omniauth identities. - if user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider) - # Case when a LDAP user already exists in Gitlab. Add the Omniauth identity to existing account. + # check if the user already has a GitLab account. + if (user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider)) + # Case when a LDAP user already exists in Gitlab. Add the OAuth identity to existing account. + log.info "LDAP account found for user #{user.username}. Building new identity." user.identities.build(extern_uid: auth_hash.uid, provider: auth_hash.provider) else - # No account in Gitlab yet: create it and add the LDAP identity - user = build_new_user + log.info 'No existing LDAP account was found in GitLab. Checking for OAuth account.' + user = find_by_uid_and_provider + if user.nil? + log.info 'No user found with the specified OAuth provider. Creating a new one.' + user = build_new_user + end + log.info "Correct account has been found. Adding LDAP identity to user: #{user.username}." user.identities.new(provider: ldap_person.provider, extern_uid: ldap_person.dn) end @@ -96,7 +102,7 @@ module Gitlab # Look for a corresponding person with same uid in any of the configured LDAP providers Gitlab::LDAP::Config.providers.each do |provider| adapter = Gitlab::LDAP::Adapter.new(provider) - @ldap_person = Gitlab::LDAP::Person.find_by_dn(auth_hash.uid, adapter) + @ldap_person = Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter) break if @ldap_person end @ldap_person diff --git a/lib/gitlab/saml/user.rb b/lib/gitlab/saml/user.rb index 6f7d4825ae..8943022612 100644 --- a/lib/gitlab/saml/user.rb +++ b/lib/gitlab/saml/user.rb @@ -62,30 +62,6 @@ module Gitlab !Gitlab::Saml::Config.external_groups.nil? end - def find_or_create_ldap_user - return unless ldap_person - - # If a corresponding person exists with same uid in a LDAP server, - # check if the user already has a GitLab account - user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider) - if user - # Case when a LDAP user already exists in Gitlab. Add the SAML identity to existing account. - user.identities.build(extern_uid: auth_hash.uid, provider: auth_hash.provider) - else - # No account found using LDAP in Gitlab yet: check if there is a SAML account with - # the passed uid and provider - user = find_by_uid_and_provider - if user.nil? - # No SAML account found, build a new user. - user = build_new_user - end - # Correct account is present, add the LDAP Identity to the user. - user.identities.new(provider: ldap_person.provider, extern_uid: ldap_person.dn) - end - - user - end - def auth_hash=(auth_hash) @auth_hash = Gitlab::Saml::AuthHash.new(auth_hash) end diff --git a/spec/lib/gitlab/saml/user_spec.rb b/spec/lib/gitlab/saml/user_spec.rb index f0a17244ff..5957998e0f 100644 --- a/spec/lib/gitlab/saml/user_spec.rb +++ b/spec/lib/gitlab/saml/user_spec.rb @@ -182,7 +182,7 @@ describe Gitlab::Saml::User, lib: true do context 'user has SAML user, and wants to add their LDAP identity' do it 'adds the LDAP identity to the existing SAML user' do create(:omniauth_user, email: 'john@mail.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'saml', username: 'john') - local_hash = OmniAuth::AuthHash.new(uid: 'uid=user1,ou=People,dc=example', provider: provider, info: info_hash, extra: { raw_info: OneLogin::RubySaml::Attributes.new({ 'groups' => %w(Developers Freelancers Designers) }) }) + local_hash = OmniAuth::AuthHash.new(uid: 'uid=user1,ou=People,dc=example', provider: provider, info: info_hash) local_saml_user = described_class.new(local_hash) local_saml_user.save From 1558876ac0c7e521dabfc2b1efe3418d34fe531f Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Tue, 7 Jun 2016 12:37:44 -0500 Subject: [PATCH 06/42] Side nav scrolls if content height taller than screen --- app/assets/stylesheets/framework/sidebar.scss | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 46d46368d2..83d6467968 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -38,6 +38,11 @@ .header-logo { height: $header-height; padding: 8px 26px; + width: $sidebar_width; + position: fixed; + z-index: 999; + overflow: hidden; + transition-duration: .3s; &:hover { background-color: #eee; @@ -73,7 +78,8 @@ .nav-sidebar { - margin: 22px 0; + margin-top: 22 + $header-height; + margin-bottom: 116px; transition-duration: .3s; list-style: none; overflow: hidden; @@ -167,6 +173,7 @@ .header-logo { width: 0; + padding: 8px 0; a { padding-left: ($sidebar_collapsed_width - 36) / 2; From f872f58738e27dff1d9a1b88ab6661239f6943cf Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Tue, 7 Jun 2016 14:16:34 -0500 Subject: [PATCH 07/42] Update admin sidebar --- app/views/layouts/nav/_admin.html.haml | 42 ++++++++++++-------------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/app/views/layouts/nav/_admin.html.haml b/app/views/layouts/nav/_admin.html.haml index f292730fe4..de2276e75e 100644 --- a/app/views/layouts/nav/_admin.html.haml +++ b/app/views/layouts/nav/_admin.html.haml @@ -2,106 +2,102 @@ = nav_link(controller: :dashboard, html_options: {class: 'home'}) do = link_to admin_root_path, title: 'Overview' do = icon('dashboard fw') - %span + .nav-link-text Overview = nav_link(controller: [:admin, :projects]) do = link_to admin_namespaces_projects_path, title: 'Projects' do = icon('cube fw') - %span + .nav-link-text Projects = nav_link(controller: :users) do = link_to admin_users_path, title: 'Users' do = icon('user fw') - %span + .nav-link-text Users = nav_link(controller: :groups) do = link_to admin_groups_path, title: 'Groups' do = icon('group fw') - %span + .nav-link-text Groups = nav_link(controller: :deploy_keys) do = link_to admin_deploy_keys_path, title: 'Deploy Keys' do = icon('key fw') - %span + .nav-link-text Deploy Keys = nav_link path: ['runners#index', 'runners#show'] do = link_to admin_runners_path, title: 'Runners' do = icon('cog fw') - %span + .nav-link-text Runners - %span.count= number_with_delimiter(Ci::Runner.count(:all)) = nav_link path: 'builds#index' do = link_to admin_builds_path, title: 'Builds' do = icon('link fw') - %span + .nav-link-text Builds - %span.count= number_with_delimiter(Ci::Build.count(:all)) = nav_link(controller: :logs) do = link_to admin_logs_path, title: 'Logs' do = icon('file-text fw') - %span + .nav-link-text Logs = nav_link(controller: :health_check) do = link_to admin_health_check_path, title: 'Health Check' do = icon('medkit fw') - %span + .nav-link-text Health Check = nav_link(controller: :broadcast_messages) do = link_to admin_broadcast_messages_path, title: 'Messages' do = icon('bullhorn fw') - %span + .nav-link-text Messages = nav_link(controller: :hooks) do = link_to admin_hooks_path, title: 'Hooks' do = icon('external-link fw') - %span + .nav-link-text Hooks = nav_link(controller: :background_jobs) do = link_to admin_background_jobs_path, title: 'Background Jobs' do = icon('cog fw') - %span + .nav-link-text Background Jobs = nav_link(controller: :appearances) do = link_to admin_appearances_path, title: 'Appearances' do = icon('image') - %span + .nav-link-text Appearance = nav_link(controller: :applications) do = link_to admin_applications_path, title: 'Applications' do = icon('cloud fw') - %span + .nav-link-text Applications = nav_link(controller: :services) do = link_to admin_application_settings_services_path, title: 'Service Templates' do = icon('copy fw') - %span + .nav-link-text Service Templates = nav_link(controller: :labels) do = link_to admin_labels_path, title: 'Labels' do = icon('tags fw') - %span + .nav-link-text Labels = nav_link(controller: :abuse_reports) do = link_to admin_abuse_reports_path, title: "Abuse Reports" do = icon('exclamation-circle fw') - %span + .nav-link-text Abuse Reports - %span.count= number_with_delimiter(AbuseReport.count(:all)) - if askimet_enabled? = nav_link(controller: :spam_logs) do = link_to admin_spam_logs_path, title: "Spam Logs" do = icon('exclamation-triangle fw') - %span + .nav-link-text Spam Logs - %span.count= number_with_delimiter(SpamLog.count(:all)) = nav_link(controller: :application_settings, html_options: { class: 'separate-item'}) do = link_to admin_application_settings_path, title: 'Settings' do = icon('cogs fw') - %span + .nav-link-text Settings From 2ffc459f34f2c259a7435121d0f18d3b1ff1a770 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Tue, 7 Jun 2016 14:17:12 -0500 Subject: [PATCH 08/42] Update explore sidebar --- app/views/layouts/nav/_explore.html.haml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/layouts/nav/_explore.html.haml b/app/views/layouts/nav/_explore.html.haml index 3b40006a0c..46fcf1545f 100644 --- a/app/views/layouts/nav/_explore.html.haml +++ b/app/views/layouts/nav/_explore.html.haml @@ -2,20 +2,20 @@ = nav_link(path: ['dashboard#show', 'root#show', 'projects#trending', 'projects#starred', 'projects#index'], html_options: {class: 'home'}) do = link_to explore_root_path, title: 'Projects' do = icon('bookmark fw') - %span + .nav-link-text Projects = nav_link(controller: [:groups, 'groups/milestones', 'groups/group_members']) do = link_to explore_groups_path, title: 'Groups' do = icon('group fw') - %span + .nav-link-text Groups = nav_link(controller: :snippets) do = link_to explore_snippets_path, title: 'Snippets' do = icon('clipboard fw') - %span + .nav-link-text Snippets = nav_link(controller: :help) do = link_to help_path, title: 'Help' do = icon('question-circle fw') - %span + .nav-link-text Help From ae736d81e1f64fe9439ac0675538d359cfdb5be9 Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Tue, 17 May 2016 17:04:01 +0100 Subject: [PATCH 09/42] Added a line to find the currently saved text and replace the markdown textarea text when cancelling Updated CHANGELOG Corrected the changes to store the markdown in a hidden input when edit starts and retrieve the original from that input when edit is cancelled Replaced hidden input with form attribute --- CHANGELOG | 1 + app/assets/javascripts/notes.js.coffee | 13 ++++++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index b9189b8088..ff5a7c22e6 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -57,6 +57,7 @@ v 8.8.4 (unreleased) - Fix importer for GitHub comments on diff - Disable Webhooks before proceeding with the GitHub import - Added descriptions to notification settings dropdown + - Markdown editor now correctly resets the input value on edit cancellation !4175 v 8.8.3 - Fix 404 page when viewing TODOs that contain milestones or labels in different projects. !4312 diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee index 8e33e915ba..ad216910c8 100644 --- a/app/assets/javascripts/notes.js.coffee +++ b/app/assets/javascripts/notes.js.coffee @@ -354,8 +354,7 @@ class @Notes Called in response to clicking the edit note link Replaces the note text with the note edit form - Adds a hidden div with the original content of the note to fill the edit note form with - if the user cancels + Adds a data attribute to the form with the original content of the note for cancellations ### showEditForm: (e, scrollTo, myLastNote) -> e.preventDefault() @@ -371,6 +370,8 @@ class @Notes done = ($noteText) -> # Neat little trick to put the cursor at the end noteTextVal = $noteText.val() + # Store the original note text in a data attribute to retrieve if a user cancels edit. + form.find('form.edit-note').data 'original-note', noteTextVal $noteText.val('').val(noteTextVal); new GLForm form @@ -393,14 +394,16 @@ class @Notes ### Called in response to clicking the edit note link - Hides edit form + Hides edit form and restores the original note text to the editor textarea. ### cancelEdit: (e) -> e.preventDefault() note = $(this).closest(".note") + form = note.find(".current-note-edit-form") note.removeClass "is-editting" - note.find(".current-note-edit-form") - .removeClass("current-note-edit-form") + form.removeClass("current-note-edit-form") + # Replace markdown textarea text with original note text. + form.find(".js-note-text").val(form.find('form.edit-note').data('original-note')) ### Called in response to deleting a note of any kind. From 8cd17f748bb564d6aeaa4fa8339b2d5431a10697 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 8 Jun 2016 12:24:10 +0100 Subject: [PATCH 10/42] Fixed dropdown line-height Most noticable on the commit header --- app/assets/stylesheets/framework/dropdowns.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 1ce7c57ebc..d4d579a083 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -124,6 +124,7 @@ position: relative; padding: 5px 10px; color: $dropdown-link-color; + line-height: initial; text-overflow: ellipsis; border-radius: 2px; white-space: nowrap; From c3024affb3d10b20928b0cce347a4dc6913a507c Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 8 Jun 2016 16:29:40 +0300 Subject: [PATCH 11/42] Use hex value instead of rgba for gray border color Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/framework/variables.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 60207ecf1d..35a75b96c5 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -121,7 +121,7 @@ $border-white-normal: #d6dae2; $border-white-dark: #c6cacf; $border-gray-light: #dcdcdc; -$border-gray-normal: rgba(0, 0, 0, 0.10); +$border-gray-normal: #d7d7d7; $border-gray-dark: #c6cacf; $border-green-light: #2faa60; From e533890bf19f6d0898ab6050f5f5ae7d061f3781 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 8 Jun 2016 16:38:18 +0300 Subject: [PATCH 12/42] Improve button color for issue and mr pages * make sidebar button white * make header button gray if not primary Signed-off-by: Dmitriy Zaporozhets --- app/views/projects/issues/show.html.haml | 8 ++++---- .../projects/merge_requests/show/_mr_title.html.haml | 6 +++--- app/views/shared/issuable/_sidebar.html.haml | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index b2f14a5407..0577f4faee 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -38,13 +38,13 @@ %li = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue) - if can?(current_user, :create_issue, @project) - = link_to new_namespace_project_issue_path(@project.namespace, @project), class: 'hidden-xs hidden-sm btn btn-nr btn-grouped new-issue-link btn-success', title: 'New issue', id: 'new_issue_link' do + = link_to new_namespace_project_issue_path(@project.namespace, @project), class: 'hidden-xs hidden-sm btn btn-grouped new-issue-link btn-success', title: 'New issue', id: 'new_issue_link' do = icon('plus') New issue - if can?(current_user, :update_issue, @issue) - = link_to 'Reopen issue', issue_path(@issue, issue: { state_event: :reopen }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-nr btn-grouped btn-reopen #{issue_button_visibility(@issue, false)}", title: 'Reopen issue' - = link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-nr btn-grouped btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue' - = link_to edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-nr btn-grouped issuable-edit' do + = link_to 'Reopen issue', issue_path(@issue, issue: { state_event: :reopen }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-gray btn-grouped btn-reopen #{issue_button_visibility(@issue, false)}", title: 'Reopen issue' + = link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-grouped btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue' + = link_to edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-gray btn-grouped issuable-edit' do = icon('pencil-square-o') Edit diff --git a/app/views/projects/merge_requests/show/_mr_title.html.haml b/app/views/projects/merge_requests/show/_mr_title.html.haml index 36c275e8be..065fabd410 100644 --- a/app/views/projects/merge_requests/show/_mr_title.html.haml +++ b/app/views/projects/merge_requests/show/_mr_title.html.haml @@ -25,8 +25,8 @@ = link_to 'Reopen', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: 'reopen-mr-link', title: 'Reopen merge request' %li = link_to 'Edit', edit_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'issuable-edit' - = link_to 'Close', merge_request_path(@merge_request, merge_request: { state_event: :close }), method: :put, class: "hidden-xs hidden-sm btn btn-nr btn-grouped btn-close #{issue_button_visibility(@merge_request, true)}", title: 'Close merge request' - = link_to 'Reopen', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: "hidden-xs hidden-sm btn btn-nr btn-grouped btn-reopen reopen-mr-link #{issue_button_visibility(@merge_request, false)}", title: 'Reopen merge request' - = link_to edit_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: "hidden-xs hidden-sm btn btn-nr btn-grouped issuable-edit" do + = link_to 'Close', merge_request_path(@merge_request, merge_request: { state_event: :close }), method: :put, class: "hidden-xs hidden-sm btn btn-grouped btn-close #{issue_button_visibility(@merge_request, true)}", title: 'Close merge request' + = link_to 'Reopen', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: "hidden-xs hidden-sm btn btn-gray btn-grouped btn-reopen reopen-mr-link #{issue_button_visibility(@merge_request, false)}", title: 'Reopen merge request' + = link_to edit_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: "hidden-xs hidden-sm btn btn-gray btn-grouped issuable-edit" do = icon('pencil-square-o') Edit diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index 1ec2436c83..c7991d53a0 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -133,7 +133,7 @@ .title.hide-collapsed Notifications - subscribtion_status = subscribed ? 'subscribed' : 'unsubscribed' - %button.btn.btn-block.btn-gray.js-subscribe-button.issuable-subscribe-button.hide-collapsed{ type: "button" } + %button.btn.btn-block.btn-default.js-subscribe-button.issuable-subscribe-button.hide-collapsed{ type: "button" } %span= subscribed ? 'Unsubscribe' : 'Subscribe' .subscription-status.hide-collapsed{data: {status: subscribtion_status}} .unsubscribed{class: ( 'hidden' if subscribed )} From 366ad9ff72b44df384bffe562ac66f85aff24c65 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 8 Jun 2016 17:11:43 +0300 Subject: [PATCH 13/42] Reject idea of using white/gray button depends on bg color Signed-off-by: Dmitriy Zaporozhets --- 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 23760a14b3..5893b7c219 100644 --- a/doc/development/ui_guide.md +++ b/doc/development/ui_guide.md @@ -49,8 +49,8 @@ information from database or file system ## Buttons * Button should contain icon or text. Exceptions should be approved by UX designer. -* Use gray button on white background or white button on gray background. * Use red button for destructive actions (not revertable). For example removing issue. * Use green or blue button for primary action. Primary button should be only one. Do not use both green and blue button in one form. +* For all other cases use default white button From 1dc001730b7d08100c0af8c210b5768c7dfbdd32 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 8 Jun 2016 17:20:07 +0300 Subject: [PATCH 14/42] Cleanup and imrpove issue/mr buttons Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/framework/buttons.scss | 15 +++++++++++++-- app/assets/stylesheets/framework/variables.scss | 3 +++ app/views/projects/issues/_new_branch.html.haml | 12 ++++++------ app/views/projects/issues/index.html.haml | 1 - app/views/projects/issues/show.html.haml | 6 ++---- app/views/projects/merge_requests/_show.html.haml | 4 +--- app/views/projects/merge_requests/index.html.haml | 1 - .../merge_requests/show/_mr_title.html.haml | 5 ++--- 8 files changed, 27 insertions(+), 20 deletions(-) diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index 467f3b35d7..625200cbca 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -142,15 +142,26 @@ } &.btn-grouped { - margin-right: 7px; + margin-right: $btn-side-margin; float: left; + + &.inline { + float: none; + } + &:last-child { margin-right: 0; } + + &.btn-sm { + margin-right: $btn-sm-side-margin; + } + &.btn-xs { - margin-right: 3px; + margin-right: $btn-xs-side-margin; } } + &.disabled { pointer-events: auto !important; } diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 35a75b96c5..d8ea07559a 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -79,6 +79,9 @@ $provider-btn-not-active-color: #4688f1; $link-underline-blue: #4a8bee; $layout-link-gray: #7e7c7c; $todo-alert-blue: #428bca; +$btn-side-margin: 7px; +$btn-sm-side-margin: 5px; +$btn-xs-side-margin: 5px; /* * Color schema diff --git a/app/views/projects/issues/_new_branch.html.haml b/app/views/projects/issues/_new_branch.html.haml index 469429ccf3..e93b7e0d66 100644 --- a/app/views/projects/issues/_new_branch.html.haml +++ b/app/views/projects/issues/_new_branch.html.haml @@ -1,13 +1,13 @@ - if can?(current_user, :push_code, @project) .pull-right #new-branch{'data-path' => can_create_branch_namespace_project_issue_path(@project.namespace, @project, @issue)} - = link_to namespace_project_branches_path(@project.namespace, @project, branch_name: @issue.to_branch_name, issue_iid: @issue.iid), method: :post, class: 'btn has-tooltip', title: @issue.to_branch_name, disabled: 'disabled' do + = link_to namespace_project_branches_path(@project.namespace, @project, branch_name: @issue.to_branch_name, issue_iid: @issue.iid), + method: :post, class: 'btn has-tooltip', title: @issue.to_branch_name, disabled: 'disabled' do .checking - %i.fa.fa-spinner.fa-spin + = icon('spinner spin') Checking branches - .available(style="display: none") - %i.fa.fa-code-fork + .available.hide New branch - .unavailable(style="display: none") - %i.fa.fa-exclamation-triangle + .unavailable.hide + = icon('exclamation-triangle') New branch unavailable diff --git a/app/views/projects/issues/index.html.haml b/app/views/projects/issues/index.html.haml index 19a6f4a91f..2a4027a6ec 100644 --- a/app/views/projects/issues/index.html.haml +++ b/app/views/projects/issues/index.html.haml @@ -15,7 +15,6 @@ = render 'shared/issuable/search_form', path: namespace_project_issues_path(@project.namespace, @project) - if can? current_user, :create_issue, @project = link_to new_namespace_project_issue_path(@project.namespace, @project, issue: { assignee_id: @issuable_finder.assignee.try(:id), milestone_id: @issuable_finder.milestones.try(:first).try(:id) }), class: "btn btn-new", title: "New Issue", id: "new_issue_link" do - = icon('plus') New Issue = render 'shared/issuable/filter', type: :issues diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index 0577f4faee..9b6a97c095 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -39,13 +39,11 @@ = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue) - if can?(current_user, :create_issue, @project) = link_to new_namespace_project_issue_path(@project.namespace, @project), class: 'hidden-xs hidden-sm btn btn-grouped new-issue-link btn-success', title: 'New issue', id: 'new_issue_link' do - = icon('plus') New issue - if can?(current_user, :update_issue, @issue) - = link_to 'Reopen issue', issue_path(@issue, issue: { state_event: :reopen }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-gray btn-grouped btn-reopen #{issue_button_visibility(@issue, false)}", title: 'Reopen issue' + = link_to 'Reopen issue', issue_path(@issue, issue: { state_event: :reopen }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-grouped btn-reopen #{issue_button_visibility(@issue, false)}", title: 'Reopen issue' = link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-grouped btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue' - = link_to edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-gray btn-grouped issuable-edit' do - = icon('pencil-square-o') + = link_to edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped issuable-edit' do Edit diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index c30459ae56..c4df8bd504 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -14,13 +14,11 @@ - if @merge_request.open? .pull-right - if @merge_request.source_branch_exists? - = link_to "#modal_merge_info", class: "btn btn-sm", "data-toggle" => "modal" do - = icon('cloud-download fw') + = link_to "#modal_merge_info", class: "btn inline btn-grouped btn-sm", "data-toggle" => "modal" do Check out branch %span.dropdown %a.btn.btn-sm.dropdown-toggle{ data: {toggle: :dropdown} } - = icon('download') Download as %span.caret %ul.dropdown-menu diff --git a/app/views/projects/merge_requests/index.html.haml b/app/views/projects/merge_requests/index.html.haml index b517e874b0..c8653cb0c3 100644 --- a/app/views/projects/merge_requests/index.html.haml +++ b/app/views/projects/merge_requests/index.html.haml @@ -10,7 +10,6 @@ - merge_project = can?(current_user, :create_merge_request, @project) ? @project : (current_user && current_user.fork_of(@project)) - if merge_project = link_to new_namespace_project_merge_request_path(merge_project.namespace, merge_project), class: "btn btn-new", title: "New Merge Request" do - = icon('plus') New Merge Request = render 'shared/issuable/filter', type: :merge_requests diff --git a/app/views/projects/merge_requests/show/_mr_title.html.haml b/app/views/projects/merge_requests/show/_mr_title.html.haml index 065fabd410..5bf5210aea 100644 --- a/app/views/projects/merge_requests/show/_mr_title.html.haml +++ b/app/views/projects/merge_requests/show/_mr_title.html.haml @@ -26,7 +26,6 @@ %li = link_to 'Edit', edit_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'issuable-edit' = link_to 'Close', merge_request_path(@merge_request, merge_request: { state_event: :close }), method: :put, class: "hidden-xs hidden-sm btn btn-grouped btn-close #{issue_button_visibility(@merge_request, true)}", title: 'Close merge request' - = link_to 'Reopen', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: "hidden-xs hidden-sm btn btn-gray btn-grouped btn-reopen reopen-mr-link #{issue_button_visibility(@merge_request, false)}", title: 'Reopen merge request' - = link_to edit_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: "hidden-xs hidden-sm btn btn-gray btn-grouped issuable-edit" do - = icon('pencil-square-o') + = link_to 'Reopen', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: "hidden-xs hidden-sm btn btn-grouped btn-reopen reopen-mr-link #{issue_button_visibility(@merge_request, false)}", title: 'Reopen merge request' + = link_to edit_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: "hidden-xs hidden-sm btn btn-grouped issuable-edit" do Edit From 3883bc05d26d930689115c6b5d8750c594420615 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Wed, 8 Jun 2016 12:27:31 +0200 Subject: [PATCH 15/42] Remove notification settings in batches This improves performance of the duplicate notification settings migration by removing duplicates in batches instead of using one big "DELETE FROM" query. The previous query would locally run over 45 minutes without even finishing. This new setup finished in a matter of seconds. Fixes #18289 --- ...remove_duplicated_notification_settings.rb | 31 +++++++++++++++++-- db/schema.rb | 4 ++- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/db/migrate/20160603180330_remove_duplicated_notification_settings.rb b/db/migrate/20160603180330_remove_duplicated_notification_settings.rb index c2fcac4c53..fe1c863b5b 100644 --- a/db/migrate/20160603180330_remove_duplicated_notification_settings.rb +++ b/db/migrate/20160603180330_remove_duplicated_notification_settings.rb @@ -1,7 +1,32 @@ class RemoveDuplicatedNotificationSettings < ActiveRecord::Migration def up - execute <<-SQL - DELETE FROM notification_settings WHERE id NOT IN ( SELECT min_id from (SELECT MIN(id) as min_id FROM notification_settings GROUP BY user_id, source_type, source_id) as dups ) - SQL + duplicates = exec_query(%Q{ + SELECT user_id, source_type, source_id + FROM notification_settings + GROUP BY user_id, source_type, source_id + HAVING COUNT(*) > 1 + }) + + duplicates.each do |row| + uid = row['user_id'] + stype = connection.quote(row['source_type']) + sid = row['source_id'] + + execute(%Q{ + DELETE FROM notification_settings + WHERE user_id = #{uid} + AND source_type = #{stype} + AND source_id = #{sid} + AND id != ( + SELECT id FROM ( + SELECT min(id) AS id + FROM notification_settings + WHERE user_id = #{uid} + AND source_type = #{stype} + AND source_id = #{sid} + ) min_ids + ) + }) + end end end diff --git a/db/schema.rb b/db/schema.rb index 69e37470de..00829d63b6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,8 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160530150109) do +ActiveRecord::Schema.define(version: 20160603182247) do + # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" enable_extension "pg_trgm" @@ -676,6 +677,7 @@ ActiveRecord::Schema.define(version: 20160530150109) do end add_index "notification_settings", ["source_id", "source_type"], name: "index_notification_settings_on_source_id_and_source_type", using: :btree + add_index "notification_settings", ["user_id", "source_id", "source_type"], name: "index_notifications_on_user_id_and_source_id_and_source_type", unique: true, using: :btree add_index "notification_settings", ["user_id"], name: "index_notification_settings_on_user_id", using: :btree create_table "oauth_access_grants", force: :cascade do |t| From ad83c3085513dd248b979d445e545e88a17c6ebc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Fri, 3 Jun 2016 14:47:09 -0400 Subject: [PATCH 16/42] Remove `projects` inclusion in `notes_with_associations` to skip some unnecessary queries `notes_with_associations` are used for `participant` declarations, but `Participable` only really cares about the target entity project, and not the participants projects. `notes_with_associations` are also used in `Commit::has_been_reverted?` which employs the reference extractor of the commit, so no references to the notes projects are made there (`Mentionable::all_references` cares only about the `author` and other `attr_mentionable`). A paralel situation occurs on `Issue::referenced_merge_requests`. --- app/models/commit.rb | 2 +- app/models/concerns/issuable.rb | 2 +- app/models/snippet.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index b5637bc4fb..d69d518fad 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -198,7 +198,7 @@ class Commit end def notes_with_associations - notes.includes(:author, :project) + notes.includes(:author) end def method_missing(m, *args, &block) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 92526a9914..58e7557fdc 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -239,7 +239,7 @@ module Issuable end def notes_with_associations - notes.includes(:author, :project) + notes.includes(:author) end def updated_tasks diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 407697b745..f8034cb5e6 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -102,7 +102,7 @@ class Snippet < ActiveRecord::Base end def notes_with_associations - notes.includes(:author, :project) + notes.includes(:author) end class << self From fade1a4cdebb4124048e9764486812627333ff95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Mon, 6 Jun 2016 16:19:39 -0400 Subject: [PATCH 17/42] Fix pseudo n+1 queries with Note and Note Authors in issuables APIs This was not a clear cut n+1 query, given that if you're directly subscribed to all issues that the API is returning you never really need to check for the notes. However, if you're subscribed to _all_ of them, then for each issuable you need to go once to `notes`, and once to `users` (for the authors). By preemtively loading notes and authors, at worst you have 1 extra query, and at best you saved 2n extra queries. We also took advantage of this preloading of notes when counting user notes. --- app/models/concerns/issuable.rb | 24 +++++++++++++++++++++--- lib/api/issues.rb | 4 ++-- lib/api/merge_requests.rb | 2 +- spec/models/concerns/issuable_spec.rb | 26 ++++++++++++++++++++++++++ 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 58e7557fdc..0ccd3474b8 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -17,7 +17,12 @@ module Issuable belongs_to :assignee, class_name: "User" belongs_to :updated_by, class_name: "User" belongs_to :milestone - has_many :notes, as: :noteable, dependent: :destroy + has_many :notes, as: :noteable, dependent: :destroy do + def authors_loaded? + # We check first if we're loaded to not load unnecesarily. + loaded? && to_a.all? { |note| note.association(:author).loaded? } + end + end has_many :label_links, as: :target, dependent: :destroy has_many :labels, through: :label_links has_many :todos, as: :target, dependent: :destroy @@ -44,6 +49,7 @@ module Issuable scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) } scope :join_project, -> { joins(:project) } + scope :inc_notes_with_associations, -> { includes(notes: :author) } scope :references_project, -> { references(:project) } scope :non_archived, -> { join_project.where(projects: { archived: false }) } @@ -179,7 +185,13 @@ module Issuable end def user_notes_count - notes.user.count + if notes.loaded? + # Use the in-memory association to select and count to avoid hitting the db + notes.to_a.count { |note| !note.system? } + else + # do the count query + notes.user.count + end end def subscribed_without_subscriptions?(user) @@ -239,7 +251,13 @@ module Issuable end def notes_with_associations - notes.includes(:author) + # If A has_many Bs, and B has_many Cs, and you do + # `A.includes(b: :c).each { |a| a.b.includes(:c) }`, sadly ActiveRecord + # will do the inclusion again. So, we check if all notes in the relation + # already have their authors loaded (possibly because the scope + # `inc_notes_with_associations` was used) and skip the inclusion if that's + # the case. + notes.authors_loaded? ? notes : notes.includes(:author) end def updated_tasks diff --git a/lib/api/issues.rb b/lib/api/issues.rb index f59a4d6c01..4c43257c48 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -51,7 +51,7 @@ module API # GET /issues?labels=foo,bar # GET /issues?labels=foo,bar&state=opened get do - issues = current_user.issues + issues = current_user.issues.inc_notes_with_associations issues = filter_issues_state(issues, params[:state]) unless params[:state].nil? issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil? issues.reorder(issuable_order_by => issuable_sort) @@ -82,7 +82,7 @@ module API # GET /projects/:id/issues?milestone=1.0.0&state=closed # GET /issues?iid=42 get ":id/issues" do - issues = user_project.issues.visible_to_user(current_user) + issues = user_project.issues.inc_notes_with_associations.visible_to_user(current_user) issues = filter_issues_state(issues, params[:state]) unless params[:state].nil? issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil? issues = filter_by_iid(issues, params[:iid]) unless params[:iid].nil? diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 2e7836dc8f..43221d5622 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -41,7 +41,7 @@ module API # get ":id/merge_requests" do authorize! :read_merge_request, user_project - merge_requests = user_project.merge_requests + merge_requests = user_project.merge_requests.inc_notes_with_associations unless params[:iid].nil? merge_requests = filter_by_iid(merge_requests, params[:iid]) diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index dd03d64f75..efbcbf72f7 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -10,6 +10,16 @@ describe Issue, "Issuable" do it { is_expected.to belong_to(:assignee) } it { is_expected.to have_many(:notes).dependent(:destroy) } it { is_expected.to have_many(:todos).dependent(:destroy) } + + context 'Notes' do + let!(:note) { create(:note, noteable: issue, project: issue.project) } + let(:scoped_issue) { Issue.includes(notes: :author).find(issue.id) } + + it 'indicates if the notes have their authors loaded' do + expect(issue.notes).not_to be_authors_loaded + expect(scoped_issue.notes).to be_authors_loaded + end + end end describe 'Included modules' do @@ -245,6 +255,22 @@ describe Issue, "Issuable" do end end + describe '#user_notes_count' do + let(:project) { create(:project) } + let(:issue1) { create(:issue, project: project) } + let(:issue2) { create(:issue, project: project) } + + before do + create_list(:note, 3, noteable: issue1, project: project) + create_list(:note, 6, noteable: issue2, project: project) + end + + it 'counts the user notes' do + expect(issue1.user_notes_count).to be(3) + expect(issue2.user_notes_count).to be(6) + end + end + describe "votes" do let(:project) { issue.project } From e7bf943127c9798d55bf59552c55e8400e47b56e Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 8 Jun 2016 16:29:09 +0200 Subject: [PATCH 18/42] Update Knapsack report only on master --- .gitlab-ci.yml | 2 ++ db/schema.rb | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5ef3081395..3dc48a8946 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -61,6 +61,8 @@ update-knapsack: - scripts/merge-reports knapsack/rspec_report.json knapsack/rspec_node_*.json - scripts/merge-reports knapsack/spinach_report.json knapsack/spinach_node_*.json - rm -f knapsack/*_node_*.json + only: + - master # Execute all testing suites diff --git a/db/schema.rb b/db/schema.rb index 69e37470de..00829d63b6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,8 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160530150109) do +ActiveRecord::Schema.define(version: 20160603182247) do + # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" enable_extension "pg_trgm" @@ -676,6 +677,7 @@ ActiveRecord::Schema.define(version: 20160530150109) do end add_index "notification_settings", ["source_id", "source_type"], name: "index_notification_settings_on_source_id_and_source_type", using: :btree + add_index "notification_settings", ["user_id", "source_id", "source_type"], name: "index_notifications_on_user_id_and_source_id_and_source_type", unique: true, using: :btree add_index "notification_settings", ["user_id"], name: "index_notification_settings_on_user_id", using: :btree create_table "oauth_access_grants", force: :cascade do |t| From bf9e482695481f5a3ec32f7c36d7ec051ec96fa5 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 8 Jun 2016 17:32:49 +0300 Subject: [PATCH 19/42] Remove unnecessary icons from buttons Signed-off-by: Dmitriy Zaporozhets --- app/views/dashboard/_groups_head.html.haml | 1 - app/views/dashboard/_projects_head.html.haml | 1 - app/views/groups/group_members/_group_member.html.haml | 4 ++-- app/views/groups/show.html.haml | 1 - app/views/projects/branches/_branch.html.haml | 2 -- app/views/projects/labels/index.html.haml | 1 - app/views/projects/project_members/_group_members.html.haml | 1 - app/views/projects/project_members/_project_member.html.haml | 4 ++-- app/views/shared/_new_project_item_select.html.haml | 3 +-- 9 files changed, 5 insertions(+), 13 deletions(-) diff --git a/app/views/dashboard/_groups_head.html.haml b/app/views/dashboard/_groups_head.html.haml index 3d17f74b70..23c145ebbb 100644 --- a/app/views/dashboard/_groups_head.html.haml +++ b/app/views/dashboard/_groups_head.html.haml @@ -9,5 +9,4 @@ - if current_user.can_create_group? .nav-controls = link_to new_group_path, class: "btn btn-new" do - = icon('plus') New Group diff --git a/app/views/dashboard/_projects_head.html.haml b/app/views/dashboard/_projects_head.html.haml index 9da3fcbd98..d35f332e1e 100644 --- a/app/views/dashboard/_projects_head.html.haml +++ b/app/views/dashboard/_projects_head.html.haml @@ -18,5 +18,4 @@ = render 'shared/projects/dropdown' - if current_user.can_create_project? = link_to new_project_path, class: 'btn btn-new' do - = icon('plus') New Project diff --git a/app/views/groups/group_members/_group_member.html.haml b/app/views/groups/group_members/_group_member.html.haml index 60234be8f8..271700e6db 100644 --- a/app/views/groups/group_members/_group_member.html.haml +++ b/app/views/groups/group_members/_group_member.html.haml @@ -36,7 +36,7 @@ - if can?(current_user, :update_group_member, member) = button_tag class: "btn-xs btn js-toggle-button", title: 'Edit access level', type: 'button' do - %i.fa.fa-pencil-square-o + = icon('pencil') - if can?(current_user, :destroy_group_member, member)   @@ -46,7 +46,7 @@ Leave - else = link_to group_group_member_path(@group, member), data: { confirm: remove_user_from_group_message(@group, member) }, method: :delete, remote: true, class: "btn-xs btn btn-remove", title: 'Remove user from group' do - %i.fa.fa-minus.fa-inverse + = icon('trash') .edit-member.hide.js-toggle-content %br diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 77c297255b..54e89a0191 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -35,7 +35,6 @@ = render 'shared/projects/dropdown' - if can? current_user, :create_projects, @group = link_to new_project_path(namespace_id: @group.id), class: 'btn btn-new pull-right' do - = icon('plus') New Project .tab-content diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml index 57e507e68c..87c732626a 100644 --- a/app/views/projects/branches/_branch.html.haml +++ b/app/views/projects/branches/_branch.html.haml @@ -21,12 +21,10 @@ .controls.hidden-xs - if create_mr_button?(@repository.root_ref, branch.name) = link_to create_mr_path(@repository.root_ref, branch.name), class: 'btn btn-grouped btn-xs' do - = icon('plus') Merge Request - if branch.name != @repository.root_ref = link_to namespace_project_compare_index_path(@project.namespace, @project, from: @repository.root_ref, to: branch.name), class: 'btn btn-grouped btn-xs', method: :post, title: "Compare" do - = icon("exchange") Compare - if can_remove_branch?(@project, branch.name) diff --git a/app/views/projects/labels/index.html.haml b/app/views/projects/labels/index.html.haml index c72eddba37..93583c9260 100644 --- a/app/views/projects/labels/index.html.haml +++ b/app/views/projects/labels/index.html.haml @@ -7,7 +7,6 @@ .nav-controls - if can?(current_user, :admin_label, @project) = link_to new_namespace_project_label_path(@project.namespace, @project), class: "btn btn-new" do - = icon('plus') New label .labels diff --git a/app/views/projects/project_members/_group_members.html.haml b/app/views/projects/project_members/_group_members.html.haml index c53033e367..6671ee2c6d 100644 --- a/app/views/projects/project_members/_group_members.html.haml +++ b/app/views/projects/project_members/_group_members.html.haml @@ -7,7 +7,6 @@ - if can?(current_user, :admin_group_member, @group) .controls = link_to group_group_members_path(@group), class: 'btn' do - = icon('pencil-square-o') Manage group members %ul.content-list - members.limit(20).each do |member| diff --git a/app/views/projects/project_members/_project_member.html.haml b/app/views/projects/project_members/_project_member.html.haml index 05bf3a7ef6..1e53b8e37d 100644 --- a/app/views/projects/project_members/_project_member.html.haml +++ b/app/views/projects/project_members/_project_member.html.haml @@ -34,7 +34,7 @@ - if can?(current_user, :update_project_member, member) = button_tag class: "btn-xs btn js-toggle-button", title: 'Edit access level', type: 'button' do - %i.fa.fa-pencil-square-o + = icon('pencil') - if can?(current_user, :destroy_project_member, member)   @@ -44,7 +44,7 @@ Leave - else = link_to namespace_project_project_member_path(@project.namespace, @project, member), data: { confirm: remove_from_project_team_message(@project, member) }, method: :delete, remote: true, class: "btn-xs btn btn-remove", title: 'Remove user from team' do - %i.fa.fa-minus.fa-inverse + = icon('trash') .edit-member.hide.js-toggle-content %br diff --git a/app/views/shared/_new_project_item_select.html.haml b/app/views/shared/_new_project_item_select.html.haml index 1c58345278..51622931e2 100644 --- a/app/views/shared/_new_project_item_select.html.haml +++ b/app/views/shared/_new_project_item_select.html.haml @@ -1,8 +1,7 @@ - if @projects.any? - .prepend-left-10.project-item-select-holder + .project-item-select-holder = project_select_tag :project_path, class: "project-item-select", data: { include_groups: local_assigns[:include_groups], order_by: 'last_activity_at' } %a.btn.btn-new.new-project-item-select-button - = icon('plus') = local_assigns[:label] %b.caret From 13d941e185c3ca1e9ad54c97f878e9cfdd32e5aa Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 8 Jun 2016 16:49:01 +0200 Subject: [PATCH 20/42] Make Omniauth providers specs to not modify global configuration --- CHANGELOG | 1 + spec/controllers/import/import_spec_helper.rb | 2 +- spec/lib/gitlab/bitbucket_import/client_spec.rb | 4 +++- spec/lib/gitlab/bitbucket_import/importer_spec.rb | 4 +++- spec/lib/gitlab/gitlab_import/client_spec.rb | 4 +++- spec/services/projects/import_service_spec.rb | 2 +- 6 files changed, 12 insertions(+), 5 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 0506854599..ca312a111f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -36,6 +36,7 @@ v 8.9.0 (unreleased) - Use downcased path to container repository as this is expected path by Docker - Projects pending deletion will render a 404 page - Measure queue duration between gitlab-workhorse and Rails + - Make Omniauth providers specs to not modify global configuration - Make authentication service for Container Registry to be compatible with < Docker 1.11 - Add Application Setting to configure Container Registry token expire delay (default 5min) - Cache assigned issue and merge request counts in sidebar nav diff --git a/spec/controllers/import/import_spec_helper.rb b/spec/controllers/import/import_spec_helper.rb index 9d7648e25a..6710962f08 100644 --- a/spec/controllers/import/import_spec_helper.rb +++ b/spec/controllers/import/import_spec_helper.rb @@ -28,6 +28,6 @@ module ImportSpecHelper app_id: 'asd123', app_secret: 'asd123' ) - Gitlab.config.omniauth.providers << provider + allow(Gitlab.config.omniauth).to receive(:providers).and_return([provider]) end end diff --git a/spec/lib/gitlab/bitbucket_import/client_spec.rb b/spec/lib/gitlab/bitbucket_import/client_spec.rb index 7718689e6d..760d66a148 100644 --- a/spec/lib/gitlab/bitbucket_import/client_spec.rb +++ b/spec/lib/gitlab/bitbucket_import/client_spec.rb @@ -1,12 +1,14 @@ require 'spec_helper' describe Gitlab::BitbucketImport::Client, lib: true do + include ImportSpecHelper + let(:token) { '123456' } let(:secret) { 'secret' } let(:client) { Gitlab::BitbucketImport::Client.new(token, secret) } before do - Gitlab.config.omniauth.providers << OpenStruct.new(app_id: "asd123", app_secret: "asd123", name: "bitbucket") + stub_omniauth_provider('bitbucket') end it 'all OAuth client options are symbols' do diff --git a/spec/lib/gitlab/bitbucket_import/importer_spec.rb b/spec/lib/gitlab/bitbucket_import/importer_spec.rb index 1a833f255a..aa00f32bec 100644 --- a/spec/lib/gitlab/bitbucket_import/importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_import/importer_spec.rb @@ -1,8 +1,10 @@ require 'spec_helper' describe Gitlab::BitbucketImport::Importer, lib: true do + include ImportSpecHelper + before do - Gitlab.config.omniauth.providers << OpenStruct.new(app_id: "asd123", app_secret: "asd123", name: "bitbucket") + stub_omniauth_provider('bitbucket') end let(:statuses) do diff --git a/spec/lib/gitlab/gitlab_import/client_spec.rb b/spec/lib/gitlab/gitlab_import/client_spec.rb index e6831e7c38..cd8e805466 100644 --- a/spec/lib/gitlab/gitlab_import/client_spec.rb +++ b/spec/lib/gitlab/gitlab_import/client_spec.rb @@ -1,11 +1,13 @@ require 'spec_helper' describe Gitlab::GitlabImport::Client, lib: true do + include ImportSpecHelper + let(:token) { '123456' } let(:client) { Gitlab::GitlabImport::Client.new(token) } before do - Gitlab.config.omniauth.providers << OpenStruct.new(app_id: "asd123", app_secret: "asd123", name: "gitlab") + stub_omniauth_provider('gitlab') end it 'all OAuth2 client options are symbols' do diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index 9d90bfceb7..068c9a1219 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -124,7 +124,7 @@ describe Projects::ImportService, services: true do } ) - Gitlab.config.omniauth.providers << provider + allow(Gitlab.config.omniauth).to receive(:providers).and_return([provider]) end end end From 9560639e817147064e3929243668e49336a4f3f5 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 8 Jun 2016 17:21:08 +0200 Subject: [PATCH 21/42] Move ImportSpecHelper to spec/support/ --- spec/{controllers/import => support}/import_spec_helper.rb | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename spec/{controllers/import => support}/import_spec_helper.rb (100%) diff --git a/spec/controllers/import/import_spec_helper.rb b/spec/support/import_spec_helper.rb similarity index 100% rename from spec/controllers/import/import_spec_helper.rb rename to spec/support/import_spec_helper.rb From 83e1274145792fbfb09557a2a202843129edf483 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Wed, 8 Jun 2016 12:05:23 -0400 Subject: [PATCH 22/42] Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 0506854599..657c6f4833 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -49,6 +49,7 @@ v 8.9.0 (unreleased) - Replace Colorize with Rainbow for coloring console output in Rake tasks. - An indicator is now displayed at the top of the comment field for confidential issues. - RepositoryCheck::SingleRepositoryWorker public and private methods are now instrumented + - Improve issuables APIs performance when accessing notes !4471 v 8.8.4 (unreleased) - Ensure branch cleanup regardless of whether the GitHub import process succeeds From a0adafddd0ce40c4ce9f052d2ee5e8ea38a2fb58 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 8 Jun 2016 18:18:49 +0200 Subject: [PATCH 23/42] Remove require_relative 'import_spec_helper' --- spec/controllers/import/bitbucket_controller_spec.rb | 1 - spec/controllers/import/fogbugz_controller_spec.rb | 1 - spec/controllers/import/github_controller_spec.rb | 1 - spec/controllers/import/gitlab_controller_spec.rb | 1 - spec/controllers/import/gitorious_controller_spec.rb | 1 - spec/controllers/import/google_code_controller_spec.rb | 1 - 6 files changed, 6 deletions(-) diff --git a/spec/controllers/import/bitbucket_controller_spec.rb b/spec/controllers/import/bitbucket_controller_spec.rb index 81c03c9059..07bf8d2d1c 100644 --- a/spec/controllers/import/bitbucket_controller_spec.rb +++ b/spec/controllers/import/bitbucket_controller_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_relative 'import_spec_helper' describe Import::BitbucketController do include ImportSpecHelper diff --git a/spec/controllers/import/fogbugz_controller_spec.rb b/spec/controllers/import/fogbugz_controller_spec.rb index 27b11267d2..5f0f6dea82 100644 --- a/spec/controllers/import/fogbugz_controller_spec.rb +++ b/spec/controllers/import/fogbugz_controller_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_relative 'import_spec_helper' describe Import::FogbugzController do include ImportSpecHelper diff --git a/spec/controllers/import/github_controller_spec.rb b/spec/controllers/import/github_controller_spec.rb index bcc713dce2..c55a3c2820 100644 --- a/spec/controllers/import/github_controller_spec.rb +++ b/spec/controllers/import/github_controller_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_relative 'import_spec_helper' describe Import::GithubController do include ImportSpecHelper diff --git a/spec/controllers/import/gitlab_controller_spec.rb b/spec/controllers/import/gitlab_controller_spec.rb index 198d006af7..e8cf6aa776 100644 --- a/spec/controllers/import/gitlab_controller_spec.rb +++ b/spec/controllers/import/gitlab_controller_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_relative 'import_spec_helper' describe Import::GitlabController do include ImportSpecHelper diff --git a/spec/controllers/import/gitorious_controller_spec.rb b/spec/controllers/import/gitorious_controller_spec.rb index 7cb1b85a46..4ae2b78e11 100644 --- a/spec/controllers/import/gitorious_controller_spec.rb +++ b/spec/controllers/import/gitorious_controller_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_relative 'import_spec_helper' describe Import::GitoriousController do include ImportSpecHelper diff --git a/spec/controllers/import/google_code_controller_spec.rb b/spec/controllers/import/google_code_controller_spec.rb index 66088139a6..4241db6e77 100644 --- a/spec/controllers/import/google_code_controller_spec.rb +++ b/spec/controllers/import/google_code_controller_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require_relative 'import_spec_helper' describe Import::GoogleCodeController do include ImportSpecHelper From f3637ed782dace291473f56967c26bde42dd9bf9 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Thu, 5 May 2016 12:00:40 -0500 Subject: [PATCH 24/42] Start adding SVG icons --- app/assets/stylesheets/framework/sidebar.scss | 8 ++++ app/helpers/appearances_helper.rb | 4 ++ app/views/layouts/nav/_dashboard.html.haml | 20 ++++++++ app/views/shared/icons/_activity.svg | 18 +++++++ .../shared/icons/_contributionanalytics.svg | 27 +++++++++++ app/views/shared/icons/_group.svg | 42 ++++++++++++++++ app/views/shared/icons/_issues.svg | 38 +++++++++++++++ app/views/shared/icons/_members.svg | 17 +++++++ app/views/shared/icons/_milestones.svg | 48 +++++++++++++++++++ app/views/shared/icons/_mr.svg | 47 ++++++++++++++++++ 10 files changed, 269 insertions(+) create mode 100644 app/views/shared/icons/_activity.svg create mode 100644 app/views/shared/icons/_contributionanalytics.svg create mode 100644 app/views/shared/icons/_group.svg create mode 100644 app/views/shared/icons/_issues.svg create mode 100644 app/views/shared/icons/_members.svg create mode 100644 app/views/shared/icons/_milestones.svg create mode 100644 app/views/shared/icons/_mr.svg diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 46d46368d2..fa9cddef0a 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -113,6 +113,14 @@ color: $gray-light; } + path { + fill: $gray-light; + } + + svg { + margin-right: 13px; + } + .nav-link-text { margin-top: 3px; font-size: 13px; diff --git a/app/helpers/appearances_helper.rb b/app/helpers/appearances_helper.rb index e0abc3a286..f240584ccb 100644 --- a/app/helpers/appearances_helper.rb +++ b/app/helpers/appearances_helper.rb @@ -30,4 +30,8 @@ module AppearancesHelper render 'shared/logo.svg' end end + + def navbar_icon(icon_name) + render "shared/icons/#{icon_name}.svg" + end end diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index df77d9cf83..3072981068 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -11,6 +11,7 @@ Todos = nav_link(path: 'dashboard#activity') do = link_to activity_dashboard_path, class: 'dashboard-shortcuts-activity', title: 'Activity' do +<<<<<<< e6daf1f899b412ded9a16674865b09f31fc7c75a = icon('dashboard fw') .nav-link-text Activity @@ -28,6 +29,25 @@ = link_to assigned_issues_dashboard_path, title: 'Issues', class: 'dashboard-shortcuts-issues' do = icon('exclamation-circle fw') .nav-link-text +======= + = navbar_icon('activity') + %span + Activity + = nav_link(controller: [:groups, 'groups/milestones', 'groups/group_members']) do + = link_to dashboard_groups_path, title: 'Groups' do + = navbar_icon('group') + %span + Groups + = nav_link(controller: 'dashboard/milestones') do + = link_to dashboard_milestones_path, title: 'Milestones' do + = navbar_icon('milestones') + %span + Milestones + = nav_link(path: 'dashboard#issues') do + = link_to assigned_issues_dashboard_path, title: 'Issues', class: 'dashboard-shortcuts-issues' do + = navbar_icon('issues') + %span +>>>>>>> Start adding SVG icons Issues = nav_link(path: 'dashboard#merge_requests') do = link_to assigned_mrs_dashboard_path, title: 'Merge Requests', class: 'dashboard-shortcuts-merge_requests' do diff --git a/app/views/shared/icons/_activity.svg b/app/views/shared/icons/_activity.svg new file mode 100644 index 0000000000..69e5afd3a4 --- /dev/null +++ b/app/views/shared/icons/_activity.svg @@ -0,0 +1,18 @@ + + + + Group 5 + Created with Sketch. + + + + + + + + + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_contributionanalytics.svg b/app/views/shared/icons/_contributionanalytics.svg new file mode 100644 index 0000000000..33edf0f932 --- /dev/null +++ b/app/views/shared/icons/_contributionanalytics.svg @@ -0,0 +1,27 @@ + + + + Pasted Image 234 + Created with Sketch. + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_group.svg b/app/views/shared/icons/_group.svg new file mode 100644 index 0000000000..6f2c530c9b --- /dev/null +++ b/app/views/shared/icons/_group.svg @@ -0,0 +1,42 @@ + + + + Pasted Image 232 + Created with Sketch. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_issues.svg b/app/views/shared/icons/_issues.svg new file mode 100644 index 0000000000..92f48b897b --- /dev/null +++ b/app/views/shared/icons/_issues.svg @@ -0,0 +1,38 @@ + + + + Pasted Image 227 + Created with Sketch. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_members.svg b/app/views/shared/icons/_members.svg new file mode 100644 index 0000000000..98635108d0 --- /dev/null +++ b/app/views/shared/icons/_members.svg @@ -0,0 +1,17 @@ + + + + Pasted Image 233 + Created with Sketch. + + + + + + + + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_milestones.svg b/app/views/shared/icons/_milestones.svg new file mode 100644 index 0000000000..f4f5b0c867 --- /dev/null +++ b/app/views/shared/icons/_milestones.svg @@ -0,0 +1,48 @@ + + + + Pasted Image 226 + Created with Sketch. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_mr.svg b/app/views/shared/icons/_mr.svg new file mode 100644 index 0000000000..c14a5c2816 --- /dev/null +++ b/app/views/shared/icons/_mr.svg @@ -0,0 +1,47 @@ + + + + Pasted Image 228 + Created with Sketch. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file From 5e9e9a1e8284a8d87780725f765a6c04ab100117 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Mon, 23 May 2016 18:35:18 -0500 Subject: [PATCH 25/42] Add SVG icons to side bar --- .../stylesheets/framework/gitlab-theme.scss | 27 ++++++++--- app/assets/stylesheets/framework/sidebar.scss | 4 -- .../shared/icons/_contributionanalytics.svg | 26 ++++------ app/views/shared/icons/_group.svg | 44 ++++------------- app/views/shared/icons/_issues.svg | 35 ++------------ app/views/shared/icons/_members.svg | 12 ++--- app/views/shared/icons/_milestones.svg | 47 +++---------------- app/views/shared/icons/_mr.svg | 44 ++--------------- 8 files changed, 60 insertions(+), 179 deletions(-) diff --git a/app/assets/stylesheets/framework/gitlab-theme.scss b/app/assets/stylesheets/framework/gitlab-theme.scss index cd2eba59f9..2540ff497f 100644 --- a/app/assets/stylesheets/framework/gitlab-theme.scss +++ b/app/assets/stylesheets/framework/gitlab-theme.scss @@ -22,17 +22,17 @@ &:hover { background-color: $color-dark; a { - color: #fff; + color: $white-light; h3 { - color: #fff; + color: $white-light; } } } } .collapse-nav a { - color: #fff; + color: $white-light; background: $color; } @@ -45,7 +45,7 @@ &:hover { background-color: $color-dark; - color: #fff; + color: $white-light; text-decoration: none; } } @@ -63,10 +63,20 @@ color: $color-light; } + path, + polygon { + fill: $color-light; + } + .count { color: $color-light; background: $color-dark; } + + svg { + position: relative; + top: 3px; + } } &.separate-item { @@ -74,7 +84,7 @@ } &.active a { - color: #fff; + color: $white-light; background: $color-dark; &.no-highlight { @@ -82,7 +92,12 @@ } i { - color: #fff + color: $white-light + } + + path, + polygon { + fill: $white-light; } } } diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index fa9cddef0a..12a342bf0b 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -113,10 +113,6 @@ color: $gray-light; } - path { - fill: $gray-light; - } - svg { margin-right: 13px; } diff --git a/app/views/shared/icons/_contributionanalytics.svg b/app/views/shared/icons/_contributionanalytics.svg index 33edf0f932..adf09a1496 100644 --- a/app/views/shared/icons/_contributionanalytics.svg +++ b/app/views/shared/icons/_contributionanalytics.svg @@ -1,27 +1,17 @@ - Pasted Image 234 + Group Created with Sketch. - - - + - + - - - - - - - - - - - - - + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_group.svg b/app/views/shared/icons/_group.svg index 6f2c530c9b..75cae0d16c 100644 --- a/app/views/shared/icons/_group.svg +++ b/app/views/shared/icons/_group.svg @@ -1,42 +1,18 @@ - Pasted Image 232 + Group Created with Sketch. - - - - - + - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_issues.svg b/app/views/shared/icons/_issues.svg index 92f48b897b..2682c27ade 100644 --- a/app/views/shared/icons/_issues.svg +++ b/app/views/shared/icons/_issues.svg @@ -1,38 +1,13 @@ - Pasted Image 227 + Group Created with Sketch. - - - - - + - - - - - - - - - - - - - - - - - - - - - - - - + + + \ No newline at end of file diff --git a/app/views/shared/icons/_members.svg b/app/views/shared/icons/_members.svg index 98635108d0..f8043b31fe 100644 --- a/app/views/shared/icons/_members.svg +++ b/app/views/shared/icons/_members.svg @@ -1,17 +1,13 @@ - Pasted Image 233 + Group Created with Sketch. - - - - - - - + + + \ No newline at end of file diff --git a/app/views/shared/icons/_milestones.svg b/app/views/shared/icons/_milestones.svg index f4f5b0c867..3d62ecc063 100644 --- a/app/views/shared/icons/_milestones.svg +++ b/app/views/shared/icons/_milestones.svg @@ -1,48 +1,15 @@ - Pasted Image 226 + Group Created with Sketch. - - - - - - + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_mr.svg b/app/views/shared/icons/_mr.svg index c14a5c2816..dd3dbcc447 100644 --- a/app/views/shared/icons/_mr.svg +++ b/app/views/shared/icons/_mr.svg @@ -1,47 +1,13 @@ - Pasted Image 228 + Group Created with Sketch. - - - - - - + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + \ No newline at end of file From 52080f74d42a62298654b8550e6dfbcd06de65a7 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Wed, 1 Jun 2016 16:54:04 -0600 Subject: [PATCH 26/42] Add new icons to group page layout nav --- app/assets/stylesheets/framework/nav.scss | 21 +++++++++++++++++++++ app/views/layouts/nav/_dashboard.html.haml | 12 +++--------- app/views/layouts/nav/_group.html.haml | 12 ++++++------ app/views/layouts/nav/_project.html.haml | 18 ++++++++++-------- app/views/shared/icons/_commits.svg | 10 ++++++++++ app/views/shared/icons/_files.svg | 17 +++++++++++++++++ app/views/shared/icons/_pipelines.svg | 10 ++++++++++ app/views/shared/icons/_project.svg | 10 ++++++++++ app/views/shared/icons/_wiki.svg | 10 ++++++++++ 9 files changed, 97 insertions(+), 23 deletions(-) create mode 100644 app/views/shared/icons/_commits.svg create mode 100644 app/views/shared/icons/_files.svg create mode 100644 app/views/shared/icons/_pipelines.svg create mode 100644 app/views/shared/icons/_project.svg create mode 100644 app/views/shared/icons/_wiki.svg diff --git a/app/assets/stylesheets/framework/nav.scss b/app/assets/stylesheets/framework/nav.scss index a811778df7..0918f67360 100644 --- a/app/assets/stylesheets/framework/nav.scss +++ b/app/assets/stylesheets/framework/nav.scss @@ -304,6 +304,19 @@ border-bottom: none; height: 51px; + svg { + position: relative; + top: 2px; + margin-right: 2px; + height: 15px; + width: auto; + + path, + polygon { + fill: $layout-link-gray; + } + } + .fade-right { @include fade(left, rgba(250, 250, 250, 0.4), $background-color); right: 0; @@ -325,9 +338,17 @@ } &.active { + a, i { color: $black; } + + svg { + path, + polygon { + fill: $black; + } + } } .badge { diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 3072981068..2f956dbbd7 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -1,7 +1,7 @@ %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('bookmark fw') + = navbar_icon('project') .nav-link-text Projects = nav_link(controller: :todos) do @@ -11,7 +11,6 @@ Todos = nav_link(path: 'dashboard#activity') do = link_to activity_dashboard_path, class: 'dashboard-shortcuts-activity', title: 'Activity' do -<<<<<<< e6daf1f899b412ded9a16674865b09f31fc7c75a = icon('dashboard fw') .nav-link-text Activity @@ -27,11 +26,8 @@ Milestones = nav_link(path: 'dashboard#issues') do = link_to assigned_issues_dashboard_path, title: 'Issues', class: 'dashboard-shortcuts-issues' do - = icon('exclamation-circle fw') - .nav-link-text -======= = navbar_icon('activity') - %span + .nav-link-text Activity = nav_link(controller: [:groups, 'groups/milestones', 'groups/group_members']) do = link_to dashboard_groups_path, title: 'Groups' do @@ -46,12 +42,10 @@ = nav_link(path: 'dashboard#issues') do = link_to assigned_issues_dashboard_path, title: 'Issues', class: 'dashboard-shortcuts-issues' do = navbar_icon('issues') - %span ->>>>>>> Start adding SVG icons Issues = nav_link(path: 'dashboard#merge_requests') do = link_to assigned_mrs_dashboard_path, title: 'Merge Requests', class: 'dashboard-shortcuts-merge_requests' do - = icon('tasks fw') + = navbar_icon('mr') .nav-link-text Merge Requests = nav_link(controller: :snippets) do diff --git a/app/views/layouts/nav/_group.html.haml b/app/views/layouts/nav/_group.html.haml index de15add361..9cbee0aa36 100644 --- a/app/views/layouts/nav/_group.html.haml +++ b/app/views/layouts/nav/_group.html.haml @@ -5,36 +5,36 @@ .fade-left = nav_link(path: 'groups#show', html_options: {class: 'home'}) do = link_to group_path(@group), title: 'Home' do - = icon('group fw') + = navbar_icon('group') %span Group = nav_link(path: 'groups#activity') do = link_to activity_group_path(@group), title: 'Activity' do - = icon('dashboard fw') + = navbar_icon('activity') %span Activity = nav_link(controller: [:group, :milestones]) do = link_to group_milestones_path(@group), title: 'Milestones' do - = icon('clock-o fw') + = navbar_icon('milestones') %span Milestones = nav_link(path: 'groups#issues') do = link_to issues_group_path(@group), title: 'Issues' do - = icon('exclamation-circle fw') + = navbar_icon('issues') %span Issues - issues = IssuesFinder.new(current_user, group_id: @group.id, state: 'opened').execute %span.badge.count= number_with_delimiter(issues.count) = nav_link(path: 'groups#merge_requests') do = link_to merge_requests_group_path(@group), title: 'Merge Requests' do - = icon('tasks fw') + = navbar_icon('mr') %span Merge Requests - merge_requests = MergeRequestsFinder.new(current_user, group_id: @group.id, state: 'opened').execute %span.badge.count= number_with_delimiter(merge_requests.count) = nav_link(controller: [:group_members]) do = link_to group_group_members_path(@group), title: 'Members' do - = icon('users fw') + = navbar_icon('members') %span Members .fade-right diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml index 03c9fa0a94..2a58ef224b 100644 --- a/app/views/layouts/nav/_project.html.haml +++ b/app/views/layouts/nav/_project.html.haml @@ -24,17 +24,19 @@ .fade-left = nav_link(path: 'projects#show', html_options: {class: 'home'}) do = link_to project_path(@project), title: 'Project', class: 'shortcuts-project' do - = icon('bookmark fw') + = navbar_icon('project') %span Project + = nav_link(path: 'projects#activity') do = link_to activity_project_path(@project), title: 'Activity', class: 'shortcuts-project-activity' do - = icon('dashboard fw') + = navbar_icon('activity') %span Activity + - if project_nav_tab? :files = nav_link(controller: %w(tree blob blame edit_tree new_tree find_file commit commits compare repositories tags branches releases network)) do - = link_to project_files_path(@project), title: 'Files', class: 'shortcuts-tree' do + = link_to project_files_path(@project), title: 'Code', class: 'shortcuts-tree' do = icon('code fw') %span Code @@ -42,7 +44,7 @@ - if project_nav_tab? :pipelines = nav_link(controller: :pipelines) do = link_to project_pipelines_path(@project), title: 'Pipelines', class: 'shortcuts-pipelines' do - = icon('ship fw') + = navbar_icon('pipelines') %span Pipelines @@ -63,14 +65,14 @@ - if project_nav_tab? :milestones = nav_link(controller: :milestones) do = link_to namespace_project_milestones_path(@project.namespace, @project), title: 'Milestones' do - = icon('clock-o fw') + = navbar_icon('milestones') %span Milestones - if project_nav_tab? :issues = nav_link(controller: :issues) do = link_to url_for_project_issues(@project, only_path: true), title: 'Issues', class: 'shortcuts-issues' do - = icon('exclamation-circle fw') + = navbar_icon('issues') %span Issues - if @project.default_issues_tracker? @@ -79,7 +81,7 @@ - if project_nav_tab? :merge_requests = nav_link(controller: :merge_requests) do = link_to namespace_project_merge_requests_path(@project.namespace, @project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do - = icon('tasks fw') + = navbar_icon('mr') %span Merge Requests %span.badge.count.merge_counter= number_with_delimiter(@project.merge_requests.opened.count) @@ -94,7 +96,7 @@ - if project_nav_tab? :wiki = nav_link(controller: :wikis) do = link_to get_project_wiki_path(@project), title: 'Wiki', class: 'shortcuts-wiki' do - = icon('book fw') + = navbar_icon('wiki') %span Wiki diff --git a/app/views/shared/icons/_commits.svg b/app/views/shared/icons/_commits.svg new file mode 100644 index 0000000000..ba9bb89935 --- /dev/null +++ b/app/views/shared/icons/_commits.svg @@ -0,0 +1,10 @@ + + + + Pasted Image 240 + Created with Sketch. + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_files.svg b/app/views/shared/icons/_files.svg new file mode 100644 index 0000000000..fc378d81e4 --- /dev/null +++ b/app/views/shared/icons/_files.svg @@ -0,0 +1,17 @@ + + + + Pasted Image 237 + Created with Sketch. + + + + + + + + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_pipelines.svg b/app/views/shared/icons/_pipelines.svg new file mode 100644 index 0000000000..794e8a2702 --- /dev/null +++ b/app/views/shared/icons/_pipelines.svg @@ -0,0 +1,10 @@ + + + + Pasted Image 246 + Created with Sketch. + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_project.svg b/app/views/shared/icons/_project.svg new file mode 100644 index 0000000000..1e8b43f8c6 --- /dev/null +++ b/app/views/shared/icons/_project.svg @@ -0,0 +1,10 @@ + + + + Page 1 + Created with Sketch. + + + + + \ No newline at end of file diff --git a/app/views/shared/icons/_wiki.svg b/app/views/shared/icons/_wiki.svg new file mode 100644 index 0000000000..182d91e23a --- /dev/null +++ b/app/views/shared/icons/_wiki.svg @@ -0,0 +1,10 @@ + + + + Pasted Image 241 + Created with Sketch. + + + + + \ No newline at end of file From 7be19db42fd51da23b1ef658263897213b624500 Mon Sep 17 00:00:00 2001 From: Alfredo Sumaran Date: Wed, 8 Jun 2016 02:02:55 -0500 Subject: [PATCH 27/42] Set target="_blank" for external links --- lib/banzai/filter/external_link_filter.rb | 3 +++ spec/features/markdown_spec.rb | 10 ++++++++++ spec/fixtures/markdown.md.erb | 2 +- 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/banzai/filter/external_link_filter.rb b/lib/banzai/filter/external_link_filter.rb index 38c4219518..27ca38e6d4 100644 --- a/lib/banzai/filter/external_link_filter.rb +++ b/lib/banzai/filter/external_link_filter.rb @@ -15,6 +15,9 @@ module Banzai next if link.start_with?(internal_url) node.set_attribute('rel', 'nofollow noreferrer') + + # Open external links on a new tab + node.set_attribute('target', '_blank') end doc diff --git a/spec/features/markdown_spec.rb b/spec/features/markdown_spec.rb index 7663d19335..cabccdacf8 100644 --- a/spec/features/markdown_spec.rb +++ b/spec/features/markdown_spec.rb @@ -173,10 +173,20 @@ describe 'GitLab Markdown', feature: true do expect(link.attr('rel')).to include('noreferrer') end + it 'adds _blank to target attribute for external links' do + link = doc.at_css('a:contains("Google")') + expect(link.attr('target')).to match('_blank') + end + it 'ignores internal link' do link = doc.at_css('a:contains("GitLab Root")') expect(link.attr('rel')).not_to match 'nofollow' end + + it 'does not set _blank to target attribute for internal links' do + link = doc.at_css('a:contains("GitLab Root")') + expect(link.attr('target')).not_to match '_blank' + end end end diff --git a/spec/fixtures/markdown.md.erb b/spec/fixtures/markdown.md.erb index 34ce7c4f03..c75d28d980 100644 --- a/spec/fixtures/markdown.md.erb +++ b/spec/fixtures/markdown.md.erb @@ -136,7 +136,7 @@ But it shouldn't autolink text inside certain tags: ### ExternalLinkFilter -External links get a `rel="nofollow"` attribute: +External links get a `rel="nofollow noreferrer"` and `target="_blank"` attributes: - [Google](https://google.com/) - [GitLab Root](<%= Gitlab.config.gitlab.url %>) From 064cff13fd7d02672695b4ac1722bbd6df35f1d4 Mon Sep 17 00:00:00 2001 From: Alfredo Sumaran Date: Wed, 8 Jun 2016 12:14:56 -0500 Subject: [PATCH 28/42] Remove obvious comment and extra line --- lib/banzai/filter/external_link_filter.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/banzai/filter/external_link_filter.rb b/lib/banzai/filter/external_link_filter.rb index 27ca38e6d4..f73ecfc941 100644 --- a/lib/banzai/filter/external_link_filter.rb +++ b/lib/banzai/filter/external_link_filter.rb @@ -15,8 +15,6 @@ module Banzai next if link.start_with?(internal_url) node.set_attribute('rel', 'nofollow noreferrer') - - # Open external links on a new tab node.set_attribute('target', '_blank') end From 27ada5aa4690affd04ada606d15a44a598184c57 Mon Sep 17 00:00:00 2001 From: Alfredo Sumaran Date: Wed, 8 Jun 2016 12:20:24 -0500 Subject: [PATCH 29/42] Combine tests for internal links --- spec/features/markdown_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/features/markdown_spec.rb b/spec/features/markdown_spec.rb index cabccdacf8..1193cae5a2 100644 --- a/spec/features/markdown_spec.rb +++ b/spec/features/markdown_spec.rb @@ -165,26 +165,26 @@ describe 'GitLab Markdown', feature: true do describe 'ExternalLinkFilter' do it 'adds nofollow to external link' do link = doc.at_css('a:contains("Google")') + expect(link.attr('rel')).to include('nofollow') end it 'adds noreferrer to external link' do link = doc.at_css('a:contains("Google")') + expect(link.attr('rel')).to include('noreferrer') end it 'adds _blank to target attribute for external links' do link = doc.at_css('a:contains("Google")') + expect(link.attr('target')).to match('_blank') end it 'ignores internal link' do link = doc.at_css('a:contains("GitLab Root")') - expect(link.attr('rel')).not_to match 'nofollow' - end - it 'does not set _blank to target attribute for internal links' do - link = doc.at_css('a:contains("GitLab Root")') + expect(link.attr('rel')).not_to match 'nofollow' expect(link.attr('target')).not_to match '_blank' end end From 26d4e633413d359c278262c9d9b2d3b94b0792e2 Mon Sep 17 00:00:00 2001 From: Alfredo Sumaran Date: Wed, 8 Jun 2016 13:02:32 -0500 Subject: [PATCH 30/42] Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 657c6f4833..3ae881da36 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -50,6 +50,7 @@ v 8.9.0 (unreleased) - An indicator is now displayed at the top of the comment field for confidential issues. - RepositoryCheck::SingleRepositoryWorker public and private methods are now instrumented - Improve issuables APIs performance when accessing notes !4471 + - External links now open in a new tab v 8.8.4 (unreleased) - Ensure branch cleanup regardless of whether the GitHub import process succeeds From 40b6d9064a2ab6c89cb54b62536fe2952c6cbca6 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 9 May 2016 16:12:53 +0100 Subject: [PATCH 31/42] Allow custom text on 'almost there' page Add a new application setting, after_sign_up_text. This is text to be rendered as Markdown and shown on the 'almost there' page after a user signs up, but before they've confirmed their account. Tweak the styles for that page so that centered lists look reasonable. --- CHANGELOG | 1 + app/assets/stylesheets/pages/confirmation.scss | 10 +++++++++- .../admin/application_settings_controller.rb | 1 + app/helpers/application_settings_helper.rb | 4 ++++ app/models/application_setting.rb | 5 ++++- app/views/admin/application_settings/_form.html.haml | 5 +++++ app/views/devise/confirmations/almost_there.haml | 3 +++ ...2_add_after_sign_up_text_to_application_settings.rb | 5 +++++ db/schema.rb | 3 ++- lib/api/entities.rb | 1 + lib/gitlab/current_settings.rb | 5 ++++- 11 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 db/migrate/20160608155312_add_after_sign_up_text_to_application_settings.rb diff --git a/CHANGELOG b/CHANGELOG index 657c6f4833..0593ce2308 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -9,6 +9,7 @@ v 8.9.0 (unreleased) - Make EmailsOnPushWorker use Sidekiq mailers queue - Fix wiki page events' webhook to point to the wiki repository - Fix issue todo not remove when leave project !4150 (Long Nguyen) + - Allow customisable text on the 'nearly there' page after a user signs up - Bump recaptcha gem to 3.0.0 to remove deprecated stoken support - Allow forking projects with restricted visibility level - Improve note validation to prevent errors when creating invalid note via API diff --git a/app/assets/stylesheets/pages/confirmation.scss b/app/assets/stylesheets/pages/confirmation.scss index 125f495d6d..292225c526 100644 --- a/app/assets/stylesheets/pages/confirmation.scss +++ b/app/assets/stylesheets/pages/confirmation.scss @@ -2,13 +2,21 @@ margin-bottom: 20px; border-bottom: 1px solid #eee; - > h1 { + > h1, h2, h3, h4, h5, h6 { font-weight: 400; } .lead { margin-bottom: 20px; } + + ul, ol { + padding-left: 0; + } + + li { + list-style-type: none; + } } .confirmation-content { diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 0a34a12e2a..f4eda864aa 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -74,6 +74,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController :two_factor_grace_period, :gravatar_enabled, :sign_in_text, + :after_sign_up_text, :help_page_text, :home_page_url, :after_sign_out_path, diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 03080d2593..55313fd835 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -15,6 +15,10 @@ module ApplicationSettingsHelper current_application_settings.sign_in_text end + def after_sign_up_text + current_application_settings.after_sign_up_text + end + def shared_runners_text current_application_settings.shared_runners_text end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 42f908aa34..a744f93791 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -113,7 +113,10 @@ class ApplicationSetting < ActiveRecord::Base signup_enabled: Settings.gitlab['signup_enabled'], signin_enabled: Settings.gitlab['signin_enabled'], gravatar_enabled: Settings.gravatar['enabled'], - sign_in_text: Settings.extra['sign_in_text'], + sign_in_text: nil, + after_sign_up_text: nil, + help_page_text: nil, + shared_runners_text: nil, restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels'], max_attachment_size: Settings.gitlab['max_attachment_size'], session_expire_delay: Settings.gitlab['session_expire_delay'], diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index f149f9eb43..c883e8f97d 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -154,6 +154,11 @@ .col-sm-10 = f.text_area :sign_in_text, class: 'form-control', rows: 4 .help-block Markdown enabled + .form-group + = f.label :after_sign_up_text, class: 'control-label col-sm-2' + .col-sm-10 + = f.text_area :after_sign_up_text, class: 'form-control', rows: 4 + .help-block Markdown enabled .form-group = f.label :help_page_text, class: 'control-label col-sm-2' .col-sm-10 diff --git a/app/views/devise/confirmations/almost_there.haml b/app/views/devise/confirmations/almost_there.haml index 3c3830a3f1..73c3a3dd2e 100644 --- a/app/views/devise/confirmations/almost_there.haml +++ b/app/views/devise/confirmations/almost_there.haml @@ -3,6 +3,9 @@ Almost there... %p.lead Please check your email to confirm your account +- if after_sign_up_text.present? + .well-confirmation.text-center + = markdown(after_sign_up_text) %p.confirmation-content.text-center No confirmation email received? Please check your spam folder or .append-bottom-20.prepend-top-20.text-center diff --git a/db/migrate/20160608155312_add_after_sign_up_text_to_application_settings.rb b/db/migrate/20160608155312_add_after_sign_up_text_to_application_settings.rb new file mode 100644 index 0000000000..89826fb96c --- /dev/null +++ b/db/migrate/20160608155312_add_after_sign_up_text_to_application_settings.rb @@ -0,0 +1,5 @@ +class AddAfterSignUpTextToApplicationSettings < ActiveRecord::Migration + def change + add_column :application_settings, :after_sign_up_text, :text + end +end diff --git a/db/schema.rb b/db/schema.rb index 00829d63b6..b7adf48fdb 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: 20160603182247) do +ActiveRecord::Schema.define(version: 20160608155312) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -84,6 +84,7 @@ ActiveRecord::Schema.define(version: 20160603182247) 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.text "after_sign_up_text" end create_table "audit_events", force: :cascade do |t| diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 66c138eb90..50d69274b2 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -351,6 +351,7 @@ module API expose :signin_enabled expose :gravatar_enabled expose :sign_in_text + expose :after_sign_up_text expose :created_at expose :updated_at expose :home_page_url diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 92c7e8b9d8..5e7532f57a 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -26,7 +26,10 @@ module Gitlab signup_enabled: Settings.gitlab['signup_enabled'], signin_enabled: Settings.gitlab['signin_enabled'], gravatar_enabled: Settings.gravatar['enabled'], - sign_in_text: Settings.extra['sign_in_text'], + sign_in_text: nil, + after_sign_up_text: nil, + help_page_text: nil, + shared_runners_text: nil, restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels'], max_attachment_size: Settings.gitlab['max_attachment_size'], session_expire_delay: Settings.gitlab['session_expire_delay'], From 0cfa368bf8cae4f9a54186241b06ef588688cfb2 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 8 Jun 2016 14:17:03 -0400 Subject: [PATCH 32/42] Fix version references in 8.8 to 8.9 update guide [ci skip] --- doc/update/8.8-to-8.9.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/update/8.8-to-8.9.md b/doc/update/8.8-to-8.9.md index 67a986ead5..f14046bb4b 100644 --- a/doc/update/8.8-to-8.9.md +++ b/doc/update/8.8-to-8.9.md @@ -120,7 +120,7 @@ will need to let gitlab-workhorse listen on a TCP port. You can do this via [/etc/default/gitlab]. [Apache templates]: https://gitlab.com/gitlab-org/gitlab-recipes/tree/master/web-server/apache -[/etc/default/gitlab]: https://gitlab.com/gitlab-org/gitlab-ce/blob/8-8-stable/lib/support/init.d/gitlab.default.example#L37 +[/etc/default/gitlab]: https://gitlab.com/gitlab-org/gitlab-ce/blob/8-9-stable/lib/support/init.d/gitlab.default.example#L37 #### Init script @@ -145,7 +145,7 @@ To make sure you didn't miss anything run a more thorough check: If all items are green, then congratulations, the upgrade is complete! -## Things went south? Revert to previous version (8.7) +## Things went south? Revert to previous version (8.8) ### 1. Revert the code to the previous version From 59e47e67ea426df14dc0eb5c25073883a7d6d99c Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Wed, 8 Jun 2016 13:50:44 -0500 Subject: [PATCH 33/42] Fix merge conflicts from removing links from sidebar --- app/assets/stylesheets/framework/sidebar.scss | 6 +----- app/views/layouts/nav/_dashboard.html.haml | 20 +++---------------- 2 files changed, 4 insertions(+), 22 deletions(-) diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 12a342bf0b..f438acfa4b 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -109,12 +109,8 @@ } i { - width: 16px; color: $gray-light; - } - - svg { - margin-right: 13px; + font-size: 16px; } .nav-link-text { diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 2f956dbbd7..b73fde7797 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -11,37 +11,23 @@ Todos = nav_link(path: 'dashboard#activity') do = link_to activity_dashboard_path, class: 'dashboard-shortcuts-activity', title: 'Activity' do - = icon('dashboard fw') - .nav-link-text - Activity - = nav_link(controller: [:groups, 'groups/milestones', 'groups/group_members']) do - = link_to dashboard_groups_path, title: 'Groups' do - = icon('group fw') - .nav-link-text - Groups - = nav_link(controller: 'dashboard/milestones') do - = link_to dashboard_milestones_path, title: 'Milestones' do - = icon('clock-o fw') - .nav-link-text - Milestones - = nav_link(path: 'dashboard#issues') do - = link_to assigned_issues_dashboard_path, title: 'Issues', class: 'dashboard-shortcuts-issues' do = navbar_icon('activity') .nav-link-text Activity = nav_link(controller: [:groups, 'groups/milestones', 'groups/group_members']) do = link_to dashboard_groups_path, title: 'Groups' do = navbar_icon('group') - %span + .nav-link-text Groups = nav_link(controller: 'dashboard/milestones') do = link_to dashboard_milestones_path, title: 'Milestones' do = navbar_icon('milestones') - %span + .nav-link-text Milestones = nav_link(path: 'dashboard#issues') do = link_to assigned_issues_dashboard_path, title: 'Issues', class: 'dashboard-shortcuts-issues' do = navbar_icon('issues') + .nav-link-text Issues = nav_link(path: 'dashboard#merge_requests') do = link_to assigned_mrs_dashboard_path, title: 'Merge Requests', class: 'dashboard-shortcuts-merge_requests' do From f367582ffe275dde4a0bd5ef281fd9e93e7586e5 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Wed, 8 Jun 2016 14:03:01 -0500 Subject: [PATCH 34/42] Use container helper instead of hard coded container --- app/views/groups/show.html.haml | 2 +- app/views/projects/_home_panel.html.haml | 2 +- app/views/projects/show.html.haml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 77c297255b..dbc3d1b07f 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -5,7 +5,7 @@ = auto_discovery_link_tag(:atom, group_url(@group, format: :atom, private_token: current_user.private_token), title: "#{@group.name} activity") .cover-block.groups-cover-block - .container-fluid.container-limited + %div{ class: (container_class) } = link_to group_icon(@group), target: '_blank' do = image_tag group_icon(@group), class: "avatar group-avatar s70" .group-info diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index f0e04a0235..f5bc1b4e40 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/app/views/projects/_home_panel.html.haml @@ -1,6 +1,6 @@ - empty_repo = @project.empty_repo? .project-home-panel.cover-block.clearfix{:class => ("empty-project" if empty_repo)} - .container-fluid.container-limited + %div{ class: (container_class) } .row .project-image-container = project_icon(@project, alt: '', class: 'project-avatar avatar s70') diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index a19c7c406a..4afa902b4e 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -13,7 +13,7 @@ = render "home_panel" .project-stats.row-content-block.second-block - .container-fluid.container-limited + %div{ class: (container_class) } %ul.nav %li = link_to project_files_path(@project) do From cba32b71bd27b747f5a667ec6ba11ff5e21362ef Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 8 Jun 2016 11:34:24 +0100 Subject: [PATCH 35/42] Allow clones from /namespace/project Redirect /namespace/project/info/refs to /namespace/project.git/info/refs, so `git clone` works on either namespace/project.git or namespace/project. --- CHANGELOG | 1 + config/routes.rb | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 5136756079..419ea3d1b5 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -17,6 +17,7 @@ v 8.9.0 (unreleased) - Redesign navigation for project pages - Fix groups API to list only user's accessible projects - Redesign account and email confirmation emails + - `git clone https://host/namespace/project` now works, in addition to using the `.git` suffix - Bump nokogiri to 1.6.8 - Use gitlab-shell v3.0.0 - Use Knapsack to evenly distribute tests across multiple nodes diff --git a/config/routes.rb b/config/routes.rb index 240dcc74b0..f1a43cbbd2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -441,6 +441,23 @@ Rails.application.routes.draw do resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only: [] do resources(:projects, constraints: { id: /[a-zA-Z.0-9_\-]+(? Date: Wed, 8 Jun 2016 18:36:43 +0200 Subject: [PATCH 36/42] Skip authenticity token checks for Git HTTP --- app/controllers/projects/git_http_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 380139a9c3..348d6cf4d9 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -1,6 +1,8 @@ class Projects::GitHttpController < Projects::ApplicationController attr_reader :user + # Git clients will not know what authenticity token to send along + skip_before_action :verify_authenticity_token skip_before_action :repository before_action :authenticate_user before_action :ensure_project_found! From a280116a058bb74f3fa29f40caf2ac962b662f49 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Wed, 8 Jun 2016 16:15:30 -0500 Subject: [PATCH 37/42] Fix activity SVG --- app/assets/stylesheets/framework/sidebar.scss | 1 - app/views/shared/icons/_activity.svg | 17 +++++++---------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index f438acfa4b..a1202374a0 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -109,7 +109,6 @@ } i { - color: $gray-light; font-size: 16px; } diff --git a/app/views/shared/icons/_activity.svg b/app/views/shared/icons/_activity.svg index 69e5afd3a4..c87794b906 100644 --- a/app/views/shared/icons/_activity.svg +++ b/app/views/shared/icons/_activity.svg @@ -1,18 +1,15 @@ - - Group 5 + + path-1 Created with Sketch. - + - - - - - - - + + + + \ No newline at end of file From 9282810fb7b6102657a0ddb2a02f71b6da22067f Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Wed, 8 Jun 2016 18:09:43 -0500 Subject: [PATCH 38/42] Syntax fixes and better logging around the `ldap_person` method. --- lib/gitlab/o_auth/user.rb | 9 +++++---- spec/lib/gitlab/saml/user_spec.rb | 3 +-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 5e52979093..78f3ecb4cb 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -70,15 +70,16 @@ module Gitlab # If a corresponding person exists with same uid in a LDAP server, # check if the user already has a GitLab account. - if (user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider)) + user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider) + if user # Case when a LDAP user already exists in Gitlab. Add the OAuth identity to existing account. - log.info "LDAP account found for user #{user.username}. Building new identity." + log.info "LDAP account found for user #{user.username}. Building new #{auth_hash.provider} identity." user.identities.build(extern_uid: auth_hash.uid, provider: auth_hash.provider) else - log.info 'No existing LDAP account was found in GitLab. Checking for OAuth account.' + log.info "No existing LDAP account was found in GitLab. Checking for #{auth_hash.provider} account." user = find_by_uid_and_provider if user.nil? - log.info 'No user found with the specified OAuth provider. Creating a new one.' + log.info "No user found using #{auth_hash.provider} provider. Creating a new one." user = build_new_user end log.info "Correct account has been found. Adding LDAP identity to user: #{user.username}." diff --git a/spec/lib/gitlab/saml/user_spec.rb b/spec/lib/gitlab/saml/user_spec.rb index 5957998e0f..84c21ceefd 100644 --- a/spec/lib/gitlab/saml/user_spec.rb +++ b/spec/lib/gitlab/saml/user_spec.rb @@ -184,9 +184,9 @@ describe Gitlab::Saml::User, lib: true do create(:omniauth_user, email: 'john@mail.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'saml', username: 'john') local_hash = OmniAuth::AuthHash.new(uid: 'uid=user1,ou=People,dc=example', provider: provider, info: info_hash) local_saml_user = described_class.new(local_hash) - local_saml_user.save local_gl_user = local_saml_user.gl_user + expect(local_gl_user).to be_valid expect(local_gl_user.identities.length).to eql 2 identities_as_hash = local_gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } } @@ -194,7 +194,6 @@ describe Gitlab::Saml::User, lib: true do { provider: 'saml', extern_uid: 'uid=user1,ou=People,dc=example' } ]) end - end end end From bdc995bf00794f96af46d92f61a940896c1404d0 Mon Sep 17 00:00:00 2001 From: Connor Shea Date: Wed, 8 Jun 2016 17:09:48 -0600 Subject: [PATCH 39/42] Shows award emoji for comments to all users who are logged in. --- app/views/projects/notes/_note.html.haml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index 5ddd0ecc4c..bcdbff0801 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -20,10 +20,11 @@ - access = note.project.team.human_max_access(note.author.id) - if access %span.note-role.hidden-xs= access - - if note_editable + - if current_user = link_to '#', title: 'Award Emoji', class: 'note-action-button note-emoji-button js-add-award js-note-emoji', data: { position: 'right' } do = icon('spinner spin') = icon('smile-o') + - if note_editable = link_to '#', title: 'Edit comment', class: 'note-action-button js-note-edit' do = icon('pencil') = link_to namespace_project_note_path(note.project.namespace, note.project, note), title: 'Remove comment', method: :delete, data: { confirm: 'Are you sure you want to remove this comment?' }, remote: true, class: 'note-action-button hidden-xs js-note-delete danger' do From 8e71c19a6940b8d82c70ee9b2550b62b5169eb54 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 8 Jun 2016 10:29:20 +0530 Subject: [PATCH 40/42] Implement the correct linking behaviour in `WikiLinkFilter`. Original Comments ================= - Linking behaves as per rules documented here: https://gitlab.com/gitlab-org/gitlab-ce/blob/16568-document-wiki-linking-behavior/doc/markdown/wiki.md - All links (to other wiki pages) are rewritten to be at the level of the app root. We can't use links relative to the current page ('./foo', 'foo', '../foo'), because they won't work in the markdown preview, where the current page is suffixed with `/edit` - Move existing `WikiLinkFilter` specs to `WikiPipeline` spec. It makes sense to run these tests on the combined output of the pipeline, rather than a single filter, since we can catch issues with conflicting filters. - Add more tests to cover the new linking @rymai's Review =============== - Classes nested under `WikiLinkFilter` should declare `WikiLinkFilter`'s inherit, so nothing changes if the nested class is loaded first. - Add a blank line after a guard clause - Use keyword arguments for the `Rewriter` constructor - Invert a condition - use `if` instead of `unless` - Inline a `let` in `WikiPipeline` spec - it was only used in a single place - Change out of date spec names - Add a comment for every rewrite rule in `Rewriter` --- lib/banzai/filter/wiki_link_filter.rb | 32 +----- .../filter/wiki_link_filter/rewriter.rb | 40 +++++++ spec/factories/wiki_pages.rb | 2 +- .../banzai/filter/wiki_link_filter_spec.rb | 85 -------------- .../lib/banzai/pipeline/wiki_pipeline_spec.rb | 108 ++++++++++++++++++ 5 files changed, 155 insertions(+), 112 deletions(-) create mode 100644 lib/banzai/filter/wiki_link_filter/rewriter.rb delete mode 100644 spec/lib/banzai/filter/wiki_link_filter_spec.rb diff --git a/lib/banzai/filter/wiki_link_filter.rb b/lib/banzai/filter/wiki_link_filter.rb index 7dc771afd7..37a2779d45 100644 --- a/lib/banzai/filter/wiki_link_filter.rb +++ b/lib/banzai/filter/wiki_link_filter.rb @@ -2,7 +2,8 @@ require 'uri' module Banzai module Filter - # HTML filter that "fixes" relative links to files in a repository. + # HTML filter that "fixes" links to pages/files in a wiki. + # Rewrite rules are documented in the `WikiPipeline` spec. # # Context options: # :project_wiki @@ -25,36 +26,15 @@ module Banzai end def process_link_attr(html_attr) - return if html_attr.blank? || file_reference?(html_attr) || hierarchical_link?(html_attr) + return if html_attr.blank? - uri = URI(html_attr.value) - if uri.relative? && uri.path.present? - html_attr.value = rebuild_wiki_uri(uri).to_s - end + html_attr.value = apply_rewrite_rules(html_attr.value) rescue URI::Error # noop end - def rebuild_wiki_uri(uri) - uri.path = ::File.join(project_wiki_base_path, uri.path) - uri - end - - def project_wiki - context[:project_wiki] - end - - def file_reference?(html_attr) - !File.extname(html_attr.value).blank? - end - - # Of the form `./link`, `../link`, or similar - def hierarchical_link?(html_attr) - html_attr.value[0] == '.' - end - - def project_wiki_base_path - project_wiki && project_wiki.wiki_base_path + def apply_rewrite_rules(link_string) + Rewriter.new(link_string, wiki: context[:project_wiki], slug: context[:page_slug]).apply_rules end end end diff --git a/lib/banzai/filter/wiki_link_filter/rewriter.rb b/lib/banzai/filter/wiki_link_filter/rewriter.rb new file mode 100644 index 0000000000..2e2c8da311 --- /dev/null +++ b/lib/banzai/filter/wiki_link_filter/rewriter.rb @@ -0,0 +1,40 @@ +module Banzai + module Filter + class WikiLinkFilter < HTML::Pipeline::Filter + class Rewriter + def initialize(link_string, wiki:, slug:) + @uri = Addressable::URI.parse(link_string) + @wiki_base_path = wiki && wiki.wiki_base_path + @slug = slug + end + + def apply_rules + apply_file_link_rules! + apply_hierarchical_link_rules! + apply_relative_link_rules! + @uri.to_s + end + + private + + # Of the form 'file.md' + def apply_file_link_rules! + @uri = Addressable::URI.join(@slug, @uri) if @uri.extname.present? + end + + # Of the form `./link`, `../link`, or similar + def apply_hierarchical_link_rules! + @uri = Addressable::URI.join(@slug, @uri) if @uri.to_s[0] == '.' + end + + # Any link _not_ of the form `http://example.com/` + def apply_relative_link_rules! + if @uri.relative? && @uri.path.present? + link = ::File.join(@wiki_base_path, @uri.path) + @uri = Addressable::URI.parse(link) + end + end + end + end + end +end diff --git a/spec/factories/wiki_pages.rb b/spec/factories/wiki_pages.rb index 938ccf2306..efa6cbe5bb 100644 --- a/spec/factories/wiki_pages.rb +++ b/spec/factories/wiki_pages.rb @@ -2,7 +2,7 @@ require 'ostruct' FactoryGirl.define do factory :wiki_page do - page = OpenStruct.new(url_path: 'some-name') + page { OpenStruct.new(url_path: 'some-name') } association :wiki, factory: :project_wiki, strategy: :build initialize_with { new(wiki, page, true) } end diff --git a/spec/lib/banzai/filter/wiki_link_filter_spec.rb b/spec/lib/banzai/filter/wiki_link_filter_spec.rb deleted file mode 100644 index 185abbb210..0000000000 --- a/spec/lib/banzai/filter/wiki_link_filter_spec.rb +++ /dev/null @@ -1,85 +0,0 @@ -require 'spec_helper' - -describe Banzai::Filter::WikiLinkFilter, lib: true do - include FilterSpecHelper - - let(:namespace) { build_stubbed(:namespace, name: "wiki_link_ns") } - let(:project) { build_stubbed(:empty_project, :public, name: "wiki_link_project", namespace: namespace) } - let(:user) { double } - let(:project_wiki) { ProjectWiki.new(project, user) } - - describe "links within the wiki (relative)" do - describe "hierarchical links to the current directory" do - it "doesn't rewrite non-file links" do - link = "Link to Page" - filtered_link = filter(link, project_wiki: project_wiki).children[0] - - expect(filtered_link.attribute('href').value).to eq('./page') - end - - it "doesn't rewrite file links" do - link = "Link to Page" - filtered_link = filter(link, project_wiki: project_wiki).children[0] - - expect(filtered_link.attribute('href').value).to eq('./page.md') - end - end - - describe "hierarchical links to the parent directory" do - it "doesn't rewrite non-file links" do - link = "Link to Page" - filtered_link = filter(link, project_wiki: project_wiki).children[0] - - expect(filtered_link.attribute('href').value).to eq('../page') - end - - it "doesn't rewrite file links" do - link = "Link to Page" - filtered_link = filter(link, project_wiki: project_wiki).children[0] - - expect(filtered_link.attribute('href').value).to eq('../page.md') - end - end - - describe "hierarchical links to a sub-directory" do - it "doesn't rewrite non-file links" do - link = "Link to Page" - filtered_link = filter(link, project_wiki: project_wiki).children[0] - - expect(filtered_link.attribute('href').value).to eq('./subdirectory/page') - end - - it "doesn't rewrite file links" do - link = "Link to Page" - filtered_link = filter(link, project_wiki: project_wiki).children[0] - - expect(filtered_link.attribute('href').value).to eq('./subdirectory/page.md') - end - end - - describe "non-hierarchical links" do - it 'rewrites non-file links to be at the scope of the wiki root' do - link = "Link to Page" - filtered_link = filter(link, project_wiki: project_wiki).children[0] - - expect(filtered_link.attribute('href').value).to match('/wiki_link_ns/wiki_link_project/wikis/page') - end - - it "doesn't rewrite file links" do - link = "Link to Page" - filtered_link = filter(link, project_wiki: project_wiki).children[0] - - expect(filtered_link.attribute('href').value).to eq('page.md') - end - end - end - - describe "links outside the wiki (absolute)" do - it "doesn't rewrite links" do - link = "Link to Page" - filtered_link = filter(link, project_wiki: project_wiki).children[0] - - expect(filtered_link.attribute('href').value).to eq('http://example.com/page') - end - end -end diff --git a/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb b/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb index 7aa1b4a3bf..ea4ab2c852 100644 --- a/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb @@ -50,4 +50,112 @@ describe Banzai::Pipeline::WikiPipeline do end end end + + describe "Links" do + let(:namespace) { build_stubbed(:namespace, name: "wiki_link_ns") } + let(:project) { build_stubbed(:empty_project, :public, name: "wiki_link_project", namespace: namespace) } + let(:project_wiki) { ProjectWiki.new(project, double(:user)) } + let(:page) { build(:wiki_page, wiki: project_wiki, page: OpenStruct.new(url_path: 'nested/twice/start-page')) } + + { "when GitLab is hosted at a root URL" => '/', + "when GitLab is hosted at a relative URL" => '/nested/relative/gitlab' }.each do |test_name, relative_url_root| + + context test_name do + before do + allow(Gitlab.config.gitlab).to receive(:relative_url_root).and_return(relative_url_root) + end + + describe "linking to pages within the wiki" do + context "when creating hierarchical links to the current directory" do + it "rewrites non-file links to be at the scope of the current directory" do + markdown = "[Page](./page)" + output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug) + + expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/twice/page\"") + end + + it "rewrites file links to be at the scope of the current directory" do + markdown = "[Link to Page](./page.md)" + output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug) + + expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/twice/page.md\"") + end + end + + context "when creating hierarchical links to the parent directory" do + it "rewrites non-file links to be at the scope of the parent directory" do + markdown = "[Link to Page](../page)" + output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug) + + expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/page\"") + end + + it "rewrites file links to be at the scope of the parent directory" do + markdown = "[Link to Page](../page.md)" + output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug) + + expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/page.md\"") + end + end + + context "when creating hierarchical links to a sub-directory" do + it "rewrites non-file links to be at the scope of the sub-directory" do + markdown = "[Link to Page](./subdirectory/page)" + output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug) + + expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/twice/subdirectory/page\"") + end + + it "rewrites file links to be at the scope of the sub-directory" do + markdown = "[Link to Page](./subdirectory/page.md)" + output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug) + + expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/twice/subdirectory/page.md\"") + end + end + + describe "when creating non-hierarchical links" do + it 'rewrites non-file links to be at the scope of the wiki root' do + markdown = "[Link to Page](page)" + output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug) + + expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/page\"") + end + + it "rewrites file links to be at the scope of the current directory" do + markdown = "[Link to Page](page.md)" + output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug) + + expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/twice/page.md\"") + end + end + + describe "when creating root links" do + it 'rewrites non-file links to be at the scope of the wiki root' do + markdown = "[Link to Page](/page)" + output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug) + + expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/page\"") + end + + it 'rewrites file links to be at the scope of the wiki root' do + markdown = "[Link to Page](/page.md)" + output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug) + + expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/page.md\"") + end + end + end + + describe "linking to pages outside the wiki (absolute)" do + it "doesn't rewrite links" do + markdown = "[Link to Page](http://example.com/page)" + output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug) + + expect(output).to include('href="http://example.com/page"') + end + end + end + end + end end From e6b1d1669b362ad4cea27ac44e89e73f4d6e92fd Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 8 Jun 2016 10:32:50 +0530 Subject: [PATCH 41/42] Hook up the updated `WikiLinkFilter` to the wiki controllers. - Need to pass in a `page_slug` to the filter, so it can rewrite based on the current page (all links are rewritten to the level of the app root). - The earlier `markdown_preview` endpoint was at the level of the wiki. We need to know the current page (for rewriting, as above), so this commit moves the endpoint to the level of a wiki page. - Fix all tests --- app/controllers/projects/wikis_controller.rb | 2 +- app/helpers/gitlab_markdown_helper.rb | 2 +- app/views/layouts/project.html.haml | 4 ++-- config/routes.rb | 2 +- features/steps/project/wiki.rb | 4 ++-- spec/features/markdown_spec.rb | 3 ++- spec/helpers/gitlab_markdown_helper_spec.rb | 3 ++- spec/support/markdown_feature.rb | 4 ++++ 8 files changed, 15 insertions(+), 9 deletions(-) diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb index 4b404eb03f..2aa6bed072 100644 --- a/app/controllers/projects/wikis_controller.rb +++ b/app/controllers/projects/wikis_controller.rb @@ -95,7 +95,7 @@ class Projects::WikisController < Projects::ApplicationController ext.analyze(text, author: current_user) render json: { - body: view_context.markdown(text, pipeline: :wiki, project_wiki: @project_wiki), + body: view_context.markdown(text, pipeline: :wiki, project_wiki: @project_wiki, page_slug: params[:id]), references: { users: ext.users.map(&:username) } diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 0a1b48af21..067a00660a 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -108,7 +108,7 @@ module GitlabMarkdownHelper def render_wiki_content(wiki_page) case wiki_page.format when :markdown - markdown(wiki_page.content, pipeline: :wiki, project_wiki: @project_wiki) + markdown(wiki_page.content, pipeline: :wiki, project_wiki: @project_wiki, page_slug: wiki_page.slug) when :asciidoc asciidoc(wiki_page.content) else diff --git a/app/views/layouts/project.html.haml b/app/views/layouts/project.html.haml index 20d6cdf724..2049b20495 100644 --- a/app/views/layouts/project.html.haml +++ b/app/views/layouts/project.html.haml @@ -5,8 +5,8 @@ - content_for :scripts_body_top do - project = @target_project || @project - - if @project_wiki - - markdown_preview_path = namespace_project_wikis_markdown_preview_path(project.namespace, project) + - if @project_wiki && @page + - markdown_preview_path = namespace_project_wiki_markdown_preview_path(project.namespace, project, params[:id]) - else - markdown_preview_path = markdown_preview_namespace_project_path(project.namespace, project) - if current_user diff --git a/config/routes.rb b/config/routes.rb index 428302d0fd..34126bbe91 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -591,7 +591,6 @@ Rails.application.routes.draw do # Order matters to give priority to these matches get '/wikis/git_access', to: 'wikis#git_access' get '/wikis/pages', to: 'wikis#pages', as: 'wiki_pages' - post '/wikis/markdown_preview', to:'wikis#markdown_preview' post '/wikis', to: 'wikis#create' get '/wikis/*id/history', to: 'wikis#history', as: 'wiki_history', constraints: WIKI_SLUG_ID @@ -600,6 +599,7 @@ Rails.application.routes.draw do get '/wikis/*id', to: 'wikis#show', as: 'wiki', constraints: WIKI_SLUG_ID delete '/wikis/*id', to: 'wikis#destroy', constraints: WIKI_SLUG_ID put '/wikis/*id', to: 'wikis#update', constraints: WIKI_SLUG_ID + post '/wikis/*id/markdown_preview', to:'wikis#markdown_preview', constraints: WIKI_SLUG_ID, as: 'wiki_markdown_preview' end resource :repository, only: [:show, :create] do diff --git a/features/steps/project/wiki.rb b/features/steps/project/wiki.rb index 9f6aed1c5b..3cbf832c72 100644 --- a/features/steps/project/wiki.rb +++ b/features/steps/project/wiki.rb @@ -97,7 +97,7 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps file = Gollum::File.new(wiki.wiki) Gollum::Wiki.any_instance.stub(:file).with("image.jpg", "master", true).and_return(file) Gollum::File.any_instance.stub(:mime_type).and_return("image/jpeg") - expect(page).to have_link('image', href: "image.jpg") + expect(page).to have_link('image', href: "#{wiki.wiki_base_path}/image.jpg") click_on "image" end @@ -113,7 +113,7 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps end step 'I click on image link' do - expect(page).to have_link('image', href: "image.jpg") + expect(page).to have_link('image', href: "#{wiki.wiki_base_path}/image.jpg") click_on "image" end diff --git a/spec/features/markdown_spec.rb b/spec/features/markdown_spec.rb index 1d892fe1a5..07275bfb3d 100644 --- a/spec/features/markdown_spec.rb +++ b/spec/features/markdown_spec.rb @@ -231,13 +231,14 @@ describe 'GitLab Markdown', feature: true do context 'wiki pipeline' do before do @project_wiki = @feat.project_wiki + @project_wiki_page = @feat.project_wiki_page file = Gollum::File.new(@project_wiki.wiki) expect(file).to receive(:path).and_return('images/example.jpg') expect(@project_wiki).to receive(:find_file).with('images/example.jpg').and_return(file) allow(@project_wiki).to receive(:wiki_base_path) { '/namespace1/gitlabhq/wikis' } - @html = markdown(@feat.raw_markdown, { pipeline: :wiki, project_wiki: @project_wiki }) + @html = markdown(@feat.raw_markdown, { pipeline: :wiki, project_wiki: @project_wiki, page_slug: @project_wiki_page.slug }) end it_behaves_like 'all pipelines' diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 13de88e2f2..ade5c3b02d 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -121,13 +121,14 @@ describe GitlabMarkdownHelper do before do @wiki = double('WikiPage') allow(@wiki).to receive(:content).and_return('wiki content') + allow(@wiki).to receive(:slug).and_return('nested/page') helper.instance_variable_set(:@project_wiki, @wiki) end it "should use Wiki pipeline for markdown files" do allow(@wiki).to receive(:format).and_return(:markdown) - expect(helper).to receive(:markdown).with('wiki content', pipeline: :wiki, project_wiki: @wiki) + expect(helper).to receive(:markdown).with('wiki content', pipeline: :wiki, project_wiki: @wiki, page_slug: "nested/page") helper.render_wiki_content(@wiki) end diff --git a/spec/support/markdown_feature.rb b/spec/support/markdown_feature.rb index 7fc6d6fcc5..a79386b5db 100644 --- a/spec/support/markdown_feature.rb +++ b/spec/support/markdown_feature.rb @@ -32,6 +32,10 @@ class MarkdownFeature @project_wiki ||= ProjectWiki.new(project, user) end + def project_wiki_page + @project_wiki_page ||= build(:wiki_page, wiki: project_wiki) + end + def issue @issue ||= create(:issue, project: project) end From 19b91e749a6320d12fb299d33f1f6440777e0e26 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 9 Jun 2016 10:16:46 +0530 Subject: [PATCH 42/42] Add #18019 to the CHANGELOG. --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index d1cde40c1c..1e51bbff4e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -16,6 +16,7 @@ v 8.9.0 (unreleased) - Add DB index on users.state - Add rake task 'gitlab:db:configure' for conditionally seeding or migrating the database - Changed the Slack build message to use the singular duration if necessary (Aran Koning) + - Links from a wiki page to other wiki pages should be rewritten as expected - Fix issues filter when ordering by milestone - Todos will display target state if issuable target is 'Closed' or 'Merged' - Fix bug when sorting issues by milestone due date and filtering by two or more labels