From 063aa5c9222da3cbbf88cb57ce18ac937b7deb26 Mon Sep 17 00:00:00 2001 From: David Date: Mon, 10 Oct 2016 13:33:48 +0000 Subject: add an other ldap configuration example --- doc/administration/auth/ldap.md | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/doc/administration/auth/ldap.md b/doc/administration/auth/ldap.md index bf7814875bf..9444357c53e 100644 --- a/doc/administration/auth/ldap.md +++ b/doc/administration/auth/ldap.md @@ -162,12 +162,32 @@ main: # 'main' is the GitLab 'provider ID' of this LDAP server EOS ``` +An other example: +```ruby +gitlab_rails['ldap_enabled'] = true +gitlab_rails['ldap_servers'] = YAML.load <<-EOS # remember to close this block with 'EOS' below +main: # 'main' is the GitLab 'provider ID' of this LDAP server + label: 'LDAP' + host: 'ldap.company.com' + port: 3288 + uid: 'sAMAccountName' + method: 'plain' # "tls" or "ssl" or "plain" + bind_dn: 'america\\momo' + password: 'MYPASSWORD' + active_directory: true + allow_username_or_email_login: true + base: 'DC=company,DC=com' + user_filter: '(&(objectclass=user)(|(samaccountname=momo)(samaccountname=toto)))' +EOS +``` + + **Source configuration** Use the same format as `gitlab_rails['ldap_servers']` for the contents under `servers:` in the example below: -``` + production: # snip... ldap: -- cgit v1.2.1 From a98497b2dbbc6e462a3b4fb458c1246de848d03c Mon Sep 17 00:00:00 2001 From: David Date: Mon, 24 Oct 2016 07:59:02 +0000 Subject: Add more precision about LDAP configuration --- doc/administration/auth/ldap.md | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/doc/administration/auth/ldap.md b/doc/administration/auth/ldap.md index 9444357c53e..9ed7cb05449 100644 --- a/doc/administration/auth/ldap.md +++ b/doc/administration/auth/ldap.md @@ -61,11 +61,15 @@ main: # 'main' is the GitLab 'provider ID' of this LDAP server # # Example: 'Paris' or 'Acme, Ltd.' label: 'LDAP' - + + # Example: 'ldap.mydomain.com' host: '_your_ldap_server' + # This port is an example, it is sometimes different but it is always an integer and not a string port: 389 uid: 'sAMAccountName' method: 'plain' # "tls" or "ssl" or "plain" + + # Examples: 'america\\momo' or 'CN=Gitlab Git,CN=Users,DC=mydomain,DC=com' bind_dn: '_the_full_dn_of_the_user_you_will_bind_with' password: '_the_password_of_the_bind_user' @@ -97,7 +101,7 @@ main: # 'main' is the GitLab 'provider ID' of this LDAP server # Base where we can search for users # - # Ex. ou=People,dc=gitlab,dc=example + # Ex. 'ou=People,dc=gitlab,dc=example' or 'DC=mydomain,DC=com' # base: '' @@ -108,6 +112,9 @@ main: # 'main' is the GitLab 'provider ID' of this LDAP server # # Note: GitLab does not support omniauth-ldap's custom filter syntax. # + # Below an example for get only specific users + # Example: '(&(objectclass=user)(|(samaccountname=momo)(samaccountname=toto)))' + # user_filter: '' # LDAP attributes that GitLab will use to create an account for the LDAP user. @@ -162,32 +169,12 @@ main: # 'main' is the GitLab 'provider ID' of this LDAP server EOS ``` -An other example: -```ruby -gitlab_rails['ldap_enabled'] = true -gitlab_rails['ldap_servers'] = YAML.load <<-EOS # remember to close this block with 'EOS' below -main: # 'main' is the GitLab 'provider ID' of this LDAP server - label: 'LDAP' - host: 'ldap.company.com' - port: 3288 - uid: 'sAMAccountName' - method: 'plain' # "tls" or "ssl" or "plain" - bind_dn: 'america\\momo' - password: 'MYPASSWORD' - active_directory: true - allow_username_or_email_login: true - base: 'DC=company,DC=com' - user_filter: '(&(objectclass=user)(|(samaccountname=momo)(samaccountname=toto)))' -EOS -``` - - **Source configuration** Use the same format as `gitlab_rails['ldap_servers']` for the contents under `servers:` in the example below: - +``` production: # snip... ldap: -- cgit v1.2.1 From 63c3ba7ae084919e237d444c942f26093ac51da0 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Tue, 22 Nov 2016 02:09:56 +0000 Subject: Created shared/visibility_select and merged with feature permissions ui Fixed up nested padding and added dynamic text js Added padding and tests --- app/assets/javascripts/dispatcher.js.es6 | 1 + app/assets/javascripts/visibility_select.js.es6 | 29 ++++++ app/assets/stylesheets/pages/projects.scss | 12 +++ app/views/projects/edit.html.haml | 111 ++++++++++----------- app/views/shared/_visibility_select.html.haml | 11 ++ ...y-level-to-public-but-project-is-not-public.yml | 4 + .../projects/settings/visibility_settings_spec.rb | 45 +++++++++ 7 files changed, 154 insertions(+), 59 deletions(-) create mode 100644 app/assets/javascripts/visibility_select.js.es6 create mode 100644 app/views/shared/_visibility_select.html.haml create mode 100644 changelogs/unreleased/24032-changed-visibility-level-to-public-but-project-is-not-public.yml create mode 100644 spec/features/projects/settings/visibility_settings_spec.rb diff --git a/app/assets/javascripts/dispatcher.js.es6 b/app/assets/javascripts/dispatcher.js.es6 index 413117c2226..237a7dab5e9 100644 --- a/app/assets/javascripts/dispatcher.js.es6 +++ b/app/assets/javascripts/dispatcher.js.es6 @@ -262,6 +262,7 @@ case 'edit': shortcut_handler = new ShortcutsNavigation(); new ProjectNew(); + new gl.VisibilitySelect(); break; case 'new': new ProjectNew(); diff --git a/app/assets/javascripts/visibility_select.js.es6 b/app/assets/javascripts/visibility_select.js.es6 new file mode 100644 index 00000000000..a2f8a2395c1 --- /dev/null +++ b/app/assets/javascripts/visibility_select.js.es6 @@ -0,0 +1,29 @@ +(() => { + const global = window.gl || (window.gl = {}); + + const VISIBILITY_DESCRIPTIONS = { + 0: 'Project access must be granted explicitly to each user.', + 10: 'Project access must be granted explicitly to each user.', + 20: 'The project can be cloned without any authentication.', + }; + + class VisibilitySelect { + constructor() { + this.visibilitySelect = document.querySelector('.js-visibility-select'); + this.helpBlock = this.visibilitySelect.querySelector('.help-block'); + this.select = this.visibilitySelect.querySelector('select'); + if (this.select) { + this.visibilityChanged(); + this.select.addEventListener('change', this.visibilityChanged.bind(this)); + } else { + this.helpBlock.textContent = this.visibilitySelect.querySelector('.js-locked').dataset.helpBlock; + } + } + + visibilityChanged() { + this.helpBlock.innerText = VISIBILITY_DESCRIPTIONS[this.select.value]; + } + } + + global.VisibilitySelect = VisibilitySelect; +})(); diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index 72b6685d940..dcb327a67c6 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -15,10 +15,22 @@ .new_project, .edit-project { + .visibility-select-container { + padding-left: 26px; + + @media(max-width: $screen-md-min) { + padding-left: 15px; + } + } + fieldset { &.features { + .header { + padding-top: $gl-vert-padding; + } + .label-light { margin-bottom: 0; } diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 38e7fc4279c..16a26f7d066 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -21,76 +21,69 @@ .form-group = f.label :default_branch, "Default Branch", class: 'label-light' = f.select(:default_branch, @project.repository.branch_names, {}, {class: 'select2 select-wide'}) - .form-group.project-visibility-level-holder - = f.label :visibility_level, class: 'label-light' do - Visibility Level - = link_to "(?)", help_page_path("public_access/public_access") - - if can_change_visibility_level?(@project, current_user) - = render('shared/visibility_radios', model_method: :visibility_level, form: f, selected_level: @project.visibility_level, form_model: @project) - - else - .info - = visibility_level_icon(@project.visibility_level) - %strong - = visibility_level_label(@project.visibility_level) - .light= visibility_level_description(@project.visibility_level, @project) - - .form-group - = render 'shared/allow_request_access', form: f - .form-group = f.label :tag_list, "Tags", class: 'label-light' = f.text_field :tag_list, value: @project.tag_list.to_s, maxlength: 2000, class: "form-control" %p.help-block Separate tags with commas. %hr - %fieldset.features.append-bottom-0 + %fieldset.append-bottom-0 %h5.prepend-top-0 - Feature Visibility - - = f.fields_for :project_feature do |feature_fields| - .form_group.prepend-top-20 - .row - .col-md-9 - = feature_fields.label :repository_access_level, "Repository", class: 'label-light' - %span.help-block Push files to be stored in this project - .col-md-3.js-repo-access-level - = project_feature_access_select(:repository_access_level) - - .col-sm-12 - .row - .col-md-9.project-feature-nested - = feature_fields.label :merge_requests_access_level, "Merge requests", class: 'label-light' - %span.help-block Submit changes to be merged upstream - .col-md-3 - = project_feature_access_select(:merge_requests_access_level) + Sharing & Permissions + .form_group.prepend-top-20 + .row.js-visibility-select + .col-md-9 + %label.label-light + = label_tag :project_visibility, 'Project Visibility', class: 'label-light' + = link_to "(?)", help_page_path("public_access/public_access") + %span.help-block + .col-md-3.visibility-select-container + = render('shared/visibility_select', model_method: :visibility_level, form: f, selected_level: @project.visibility_level) + = f.fields_for :project_feature do |feature_fields| + %fieldset.features + .row.project-feature-nested + .col-md-9.header + = feature_fields.label :repository_access_level, "Repository", class: 'label-light' + %span.help-block Push files to be stored in this project + .col-md-3.js-repo-access-level + = project_feature_access_select(:repository_access_level) - .row - .col-md-9.project-feature-nested - = feature_fields.label :builds_access_level, "Builds", class: 'label-light' - %span.help-block Submit, test and deploy your changes before merge - .col-md-3 - = project_feature_access_select(:builds_access_level) + .col-sm-12 + .row.project-feature-nested + .col-md-9.header + = feature_fields.label :merge_requests_access_level, "Merge requests", class: 'label-light' + %span.help-block Submit changes to be merged upstream + .col-md-3 + = project_feature_access_select(:merge_requests_access_level) - .row - .col-md-9 - = feature_fields.label :snippets_access_level, "Snippets", class: 'label-light' - %span.help-block Share code pastes with others out of Git repository - .col-md-3 - = project_feature_access_select(:snippets_access_level) + .row.project-feature-nested + .col-md-9.header + = feature_fields.label :builds_access_level, "Builds", class: 'label-light' + %span.help-block Submit, test and deploy your changes before merge + .col-md-3.double-nested + = project_feature_access_select(:builds_access_level) - .row - .col-md-9 - = feature_fields.label :issues_access_level, "Issues", class: 'label-light' - %span.help-block Lightweight issue tracking system for this project - .col-md-3 - = project_feature_access_select(:issues_access_level) + .row + .col-md-9.header + = feature_fields.label :snippets_access_level, "Snippets", class: 'label-light' + %span.help-block Share code pastes with others out of Git repository + .col-md-3 + = project_feature_access_select(:snippets_access_level) - .row - .col-md-9 - = feature_fields.label :wiki_access_level, "Wiki", class: 'label-light' - %span.help-block Pages for project documentation - .col-md-3 - = project_feature_access_select(:wiki_access_level) + .row + .col-md-9.header + = feature_fields.label :issues_access_level, "Issues", class: 'label-light' + %span.help-block Lightweight issue tracking system for this project + .col-md-3 + = project_feature_access_select(:issues_access_level) + .row + .col-md-9.header + = feature_fields.label :wiki_access_level, "Wiki", class: 'label-light' + %span.help-block Pages for project documentation + .col-md-3 + = project_feature_access_select(:wiki_access_level) + .form-group + = render 'shared/allow_request_access', form: f - if Gitlab.config.lfs.enabled && current_user.admin? .row .col-md-9 diff --git a/app/views/shared/_visibility_select.html.haml b/app/views/shared/_visibility_select.html.haml new file mode 100644 index 00000000000..42210f565f6 --- /dev/null +++ b/app/views/shared/_visibility_select.html.haml @@ -0,0 +1,11 @@ +- if can_change_visibility_level?(@project, current_user) + - levels_options_hash = {} + - Gitlab::VisibilityLevel.values.each do |level| + - levels_options_hash[visibility_level_label(level)] = level + - options = options_for_select(levels_options_hash, selected_level) + = form.select(model_method, options, {}, class: 'form-control visibility-select') +- else + .info.js-locked{ data: { help_block: visibility_level_description(@project.visibility_level, @project)}} + = visibility_level_icon(@project.visibility_level) + %strong + = visibility_level_label(@project.visibility_level) diff --git a/changelogs/unreleased/24032-changed-visibility-level-to-public-but-project-is-not-public.yml b/changelogs/unreleased/24032-changed-visibility-level-to-public-but-project-is-not-public.yml new file mode 100644 index 00000000000..875106d7dd5 --- /dev/null +++ b/changelogs/unreleased/24032-changed-visibility-level-to-public-but-project-is-not-public.yml @@ -0,0 +1,4 @@ +--- +title: Updated project visibility settings UX +merge_request: 7645 +author: diff --git a/spec/features/projects/settings/visibility_settings_spec.rb b/spec/features/projects/settings/visibility_settings_spec.rb new file mode 100644 index 00000000000..86cc3fb886e --- /dev/null +++ b/spec/features/projects/settings/visibility_settings_spec.rb @@ -0,0 +1,45 @@ +require 'spec_helper' +require 'byebug' + +feature 'Visibility settings', feature: true, js: true do + let(:user) { create(:user) } + let(:project) { create(:project, namespace: user.namespace, visibility_level: 20) } + + context 'as owner' do + before do + login_as(user) + visit edit_namespace_project_path(project.namespace, project) + end + + scenario 'project visibility select is available' do + visibility_select_container = find('.js-visibility-select') + expect(visibility_select_container.find('.visibility-select').value).to eq project.visibility_level.to_s + expect(visibility_select_container).to have_content 'The project can be cloned without any authentication.' + end + + scenario 'project visibility description updates on change' do + visibility_select_container = find('.js-visibility-select') + visibility_select = visibility_select_container.find('.visibility-select') + visibility_select.select('Private') + expect(visibility_select.value).to eq '0' + expect(visibility_select_container).to have_content 'Project access must be granted explicitly to each user.' + end + end + + context 'as master' do + let(:master_user) { create(:user) } + + before do + project.team << [master_user, :master] + login_as(master_user) + visit edit_namespace_project_path(project.namespace, project) + end + + scenario 'project visibility is locked' do + visibility_select_container = find('.js-visibility-select') + expect(visibility_select_container).not_to have_select '.visibility-select' + expect(visibility_select_container).to have_content 'Public' + expect(visibility_select_container).to have_content 'The project can be cloned without any authentication.' + end + end +end -- cgit v1.2.1 From 26b0fe8d2e16adf4106859ed578dd2352cde412b Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Tue, 22 Nov 2016 07:51:49 -0700 Subject: Align permissions options --- app/assets/javascripts/visibility_select.js.es6 | 2 +- app/assets/stylesheets/pages/projects.scss | 36 ++++++------- app/views/projects/edit.html.haml | 67 ++++++++++++------------- 3 files changed, 49 insertions(+), 56 deletions(-) diff --git a/app/assets/javascripts/visibility_select.js.es6 b/app/assets/javascripts/visibility_select.js.es6 index a2f8a2395c1..e846e7ead77 100644 --- a/app/assets/javascripts/visibility_select.js.es6 +++ b/app/assets/javascripts/visibility_select.js.es6 @@ -3,7 +3,7 @@ const VISIBILITY_DESCRIPTIONS = { 0: 'Project access must be granted explicitly to each user.', - 10: 'Project access must be granted explicitly to each user.', + 10: 'This project can be cloned by any logged in user.', 20: 'The project can be cloned without any authentication.', }; diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index dcb327a67c6..74b058ac94a 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -15,29 +15,17 @@ .new_project, .edit-project { - .visibility-select-container { - padding-left: 26px; - - @media(max-width: $screen-md-min) { - padding-left: 15px; + .sharing-and-permissions { + .header { + padding-top: $gl-vert-padding; } - } - - fieldset { - - &.features { - .header { - padding-top: $gl-vert-padding; - } - - .label-light { - margin-bottom: 0; - } + .label-light { + margin-bottom: 0; + } - .help-block { - margin-top: 0; - } + .help-block { + margin-top: 0; } .form-group { @@ -876,10 +864,16 @@ pre.light-well { } } -.project-feature-nested { +.project-feature { @media (min-width: $screen-sm-min) { padding-left: 45px; } + + &.nested { + @media (min-width: $screen-sm-min) { + padding-left: 90px; + } + } } .project-repo-select { diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 16a26f7d066..ff0da46a31d 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -29,7 +29,7 @@ %fieldset.append-bottom-0 %h5.prepend-top-0 Sharing & Permissions - .form_group.prepend-top-20 + .form_group.prepend-top-20.sharing-and-permissions .row.js-visibility-select .col-md-9 %label.label-light @@ -40,48 +40,47 @@ = render('shared/visibility_select', model_method: :visibility_level, form: f, selected_level: @project.visibility_level) = f.fields_for :project_feature do |feature_fields| %fieldset.features - .row.project-feature-nested - .col-md-9.header + .row + .col-md-9.project-feature = feature_fields.label :repository_access_level, "Repository", class: 'label-light' %span.help-block Push files to be stored in this project .col-md-3.js-repo-access-level = project_feature_access_select(:repository_access_level) - .col-sm-12 - .row.project-feature-nested - .col-md-9.header - = feature_fields.label :merge_requests_access_level, "Merge requests", class: 'label-light' - %span.help-block Submit changes to be merged upstream - .col-md-3 - = project_feature_access_select(:merge_requests_access_level) + .row + .col-md-9.project-feature.nested + = feature_fields.label :merge_requests_access_level, "Merge requests", class: 'label-light' + %span.help-block Submit changes to be merged upstream + .col-md-3 + = project_feature_access_select(:merge_requests_access_level) - .row.project-feature-nested - .col-md-9.header - = feature_fields.label :builds_access_level, "Builds", class: 'label-light' - %span.help-block Submit, test and deploy your changes before merge - .col-md-3.double-nested - = project_feature_access_select(:builds_access_level) + .row + .col-md-9.project-feature.nested + = feature_fields.label :builds_access_level, "Builds", class: 'label-light' + %span.help-block Submit, test and deploy your changes before merge + .col-md-3 + = project_feature_access_select(:builds_access_level) - .row - .col-md-9.header - = feature_fields.label :snippets_access_level, "Snippets", class: 'label-light' - %span.help-block Share code pastes with others out of Git repository - .col-md-3 - = project_feature_access_select(:snippets_access_level) + .row + .col-md-9.project-feature + = feature_fields.label :snippets_access_level, "Snippets", class: 'label-light' + %span.help-block Share code pastes with others out of Git repository + .col-md-3 + = project_feature_access_select(:snippets_access_level) - .row - .col-md-9.header - = feature_fields.label :issues_access_level, "Issues", class: 'label-light' - %span.help-block Lightweight issue tracking system for this project - .col-md-3 - = project_feature_access_select(:issues_access_level) + .row + .col-md-9.project-feature + = feature_fields.label :issues_access_level, "Issues", class: 'label-light' + %span.help-block Lightweight issue tracking system for this project + .col-md-3 + = project_feature_access_select(:issues_access_level) - .row - .col-md-9.header - = feature_fields.label :wiki_access_level, "Wiki", class: 'label-light' - %span.help-block Pages for project documentation - .col-md-3 - = project_feature_access_select(:wiki_access_level) + .row + .col-md-9.project-feature + = feature_fields.label :wiki_access_level, "Wiki", class: 'label-light' + %span.help-block Pages for project documentation + .col-md-3 + = project_feature_access_select(:wiki_access_level) .form-group = render 'shared/allow_request_access', form: f - if Gitlab.config.lfs.enabled && current_user.admin? -- cgit v1.2.1 From 2c9bb135057f4fea43aa0be5b94354f288d5070f Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Tue, 22 Nov 2016 17:16:30 +0000 Subject: Fixed tests Grab permissions description from backend Review changes Added unit tests --- app/assets/javascripts/dispatcher.js.es6 | 1 - app/assets/javascripts/project_new.js | 10 +++ app/assets/javascripts/visibility_select.js.es6 | 32 ++++--- app/assets/stylesheets/pages/projects.scss | 2 + app/helpers/projects_helper.rb | 11 +++ app/views/projects/_visibility_select.html.haml | 7 ++ app/views/projects/edit.html.haml | 4 +- app/views/shared/_visibility_select.html.haml | 11 --- features/steps/shared/project.rb | 2 +- .../projects/settings/visibility_settings_spec.rb | 4 +- spec/javascripts/visibility_select_spec.js.es6 | 100 +++++++++++++++++++++ 11 files changed, 151 insertions(+), 33 deletions(-) create mode 100644 app/views/projects/_visibility_select.html.haml delete mode 100644 app/views/shared/_visibility_select.html.haml create mode 100644 spec/javascripts/visibility_select_spec.js.es6 diff --git a/app/assets/javascripts/dispatcher.js.es6 b/app/assets/javascripts/dispatcher.js.es6 index 237a7dab5e9..413117c2226 100644 --- a/app/assets/javascripts/dispatcher.js.es6 +++ b/app/assets/javascripts/dispatcher.js.es6 @@ -262,7 +262,6 @@ case 'edit': shortcut_handler = new ShortcutsNavigation(); new ProjectNew(); - new gl.VisibilitySelect(); break; case 'new': new ProjectNew(); diff --git a/app/assets/javascripts/project_new.js b/app/assets/javascripts/project_new.js index 7fc611d0dad..86d57d97eae 100644 --- a/app/assets/javascripts/project_new.js +++ b/app/assets/javascripts/project_new.js @@ -14,11 +14,21 @@ return $('.save-project-loader').show(); }; })(this)); + + this.initVisibilitySelect(); + this.toggleSettings(); this.toggleSettingsOnclick(); this.toggleRepoVisibility(); } + ProjectNew.prototype.initVisibilitySelect = function() { + const visibilityContainer = document.querySelector('.js-visibility-select'); + if (!visibilityContainer) return; + const visibilitySelect = new gl.VisibilitySelect(visibilityContainer); + visibilitySelect.init(); + }; + ProjectNew.prototype.toggleSettings = function() { var self = this; diff --git a/app/assets/javascripts/visibility_select.js.es6 b/app/assets/javascripts/visibility_select.js.es6 index e846e7ead77..f712d7ba930 100644 --- a/app/assets/javascripts/visibility_select.js.es6 +++ b/app/assets/javascripts/visibility_select.js.es6 @@ -1,29 +1,27 @@ (() => { - const global = window.gl || (window.gl = {}); - - const VISIBILITY_DESCRIPTIONS = { - 0: 'Project access must be granted explicitly to each user.', - 10: 'This project can be cloned by any logged in user.', - 20: 'The project can be cloned without any authentication.', - }; + const gl = window.gl || (window.gl = {}); class VisibilitySelect { - constructor() { - this.visibilitySelect = document.querySelector('.js-visibility-select'); - this.helpBlock = this.visibilitySelect.querySelector('.help-block'); - this.select = this.visibilitySelect.querySelector('select'); + constructor(container) { + if (!container) throw new Error('VisibilitySelect requires a container element as argument 1'); + this.container = container; + this.helpBlock = this.container.querySelector('.help-block'); + this.select = this.container.querySelector('select'); + } + + init() { if (this.select) { - this.visibilityChanged(); - this.select.addEventListener('change', this.visibilityChanged.bind(this)); + this.updateHelpText(); + this.select.addEventListener('change', this.updateHelpText.bind(this)); } else { - this.helpBlock.textContent = this.visibilitySelect.querySelector('.js-locked').dataset.helpBlock; + this.helpBlock.textContent = this.container.querySelector('.js-locked').dataset.helpBlock; } } - visibilityChanged() { - this.helpBlock.innerText = VISIBILITY_DESCRIPTIONS[this.select.value]; + updateHelpText() { + this.helpBlock.textContent = this.select.querySelector('option:checked').dataset.description; } } - global.VisibilitySelect = VisibilitySelect; + gl.VisibilitySelect = VisibilitySelect; })(); diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index 74b058ac94a..726e5c115b8 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -865,6 +865,8 @@ pre.light-well { } .project-feature { + padding-top: 10px; + @media (min-width: $screen-sm-min) { padding-left: 45px; } diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 9cda3b78761..e5438f4775b 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -445,4 +445,15 @@ module ProjectsHelper def project_issues(project) IssuesFinder.new(current_user, project_id: project.id).execute end + + def visibility_select_options(project, selected_level) + levels_options_array = Gitlab::VisibilityLevel.values.map do |level| + [ + visibility_level_label(level), + { data: { description: visibility_level_description(level, project) } }, + level + ] + end + options_for_select(levels_options_array, selected_level) + end end diff --git a/app/views/projects/_visibility_select.html.haml b/app/views/projects/_visibility_select.html.haml new file mode 100644 index 00000000000..65fc0a36ca9 --- /dev/null +++ b/app/views/projects/_visibility_select.html.haml @@ -0,0 +1,7 @@ +- if can_change_visibility_level?(@project, current_user) + = form.select(model_method, visibility_select_options(@project, selected_level), {}, class: 'form-control visibility-select') +- else + .info.js-locked{ data: { help_block: visibility_level_description(@project.visibility_level, @project) } } + = visibility_level_icon(@project.visibility_level) + %strong + = visibility_level_label(@project.visibility_level) diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index ff0da46a31d..1d7fdf68cb3 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -37,13 +37,13 @@ = link_to "(?)", help_page_path("public_access/public_access") %span.help-block .col-md-3.visibility-select-container - = render('shared/visibility_select', model_method: :visibility_level, form: f, selected_level: @project.visibility_level) + = render('projects/visibility_select', model_method: :visibility_level, form: f, selected_level: @project.visibility_level) = f.fields_for :project_feature do |feature_fields| %fieldset.features .row .col-md-9.project-feature = feature_fields.label :repository_access_level, "Repository", class: 'label-light' - %span.help-block Push files to be stored in this project + %span.help-block View and edit files in this project .col-md-3.js-repo-access-level = project_feature_access_select(:repository_access_level) diff --git a/app/views/shared/_visibility_select.html.haml b/app/views/shared/_visibility_select.html.haml deleted file mode 100644 index 42210f565f6..00000000000 --- a/app/views/shared/_visibility_select.html.haml +++ /dev/null @@ -1,11 +0,0 @@ -- if can_change_visibility_level?(@project, current_user) - - levels_options_hash = {} - - Gitlab::VisibilityLevel.values.each do |level| - - levels_options_hash[visibility_level_label(level)] = level - - options = options_for_select(levels_options_hash, selected_level) - = form.select(model_method, options, {}, class: 'form-control visibility-select') -- else - .info.js-locked{ data: { help_block: visibility_level_description(@project.visibility_level, @project)}} - = visibility_level_icon(@project.visibility_level) - %strong - = visibility_level_label(@project.visibility_level) diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index b51152c79c6..5e2c5c6232e 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -97,7 +97,7 @@ module SharedProject step 'I should see project settings' do expect(current_path).to eq edit_namespace_project_path(@project.namespace, @project) expect(page).to have_content("Project name") - expect(page).to have_content("Feature Visibility") + expect(page).to have_content("Sharing & Permissions") end def current_project diff --git a/spec/features/projects/settings/visibility_settings_spec.rb b/spec/features/projects/settings/visibility_settings_spec.rb index 86cc3fb886e..cef315ac9cd 100644 --- a/spec/features/projects/settings/visibility_settings_spec.rb +++ b/spec/features/projects/settings/visibility_settings_spec.rb @@ -1,5 +1,4 @@ require 'spec_helper' -require 'byebug' feature 'Visibility settings', feature: true, js: true do let(:user) { create(:user) } @@ -13,6 +12,7 @@ feature 'Visibility settings', feature: true, js: true do scenario 'project visibility select is available' do visibility_select_container = find('.js-visibility-select') + expect(visibility_select_container.find('.visibility-select').value).to eq project.visibility_level.to_s expect(visibility_select_container).to have_content 'The project can be cloned without any authentication.' end @@ -21,6 +21,7 @@ feature 'Visibility settings', feature: true, js: true do visibility_select_container = find('.js-visibility-select') visibility_select = visibility_select_container.find('.visibility-select') visibility_select.select('Private') + expect(visibility_select.value).to eq '0' expect(visibility_select_container).to have_content 'Project access must be granted explicitly to each user.' end @@ -37,6 +38,7 @@ feature 'Visibility settings', feature: true, js: true do scenario 'project visibility is locked' do visibility_select_container = find('.js-visibility-select') + expect(visibility_select_container).not_to have_select '.visibility-select' expect(visibility_select_container).to have_content 'Public' expect(visibility_select_container).to have_content 'The project can be cloned without any authentication.' diff --git a/spec/javascripts/visibility_select_spec.js.es6 b/spec/javascripts/visibility_select_spec.js.es6 new file mode 100644 index 00000000000..b21f6912e06 --- /dev/null +++ b/spec/javascripts/visibility_select_spec.js.es6 @@ -0,0 +1,100 @@ +/*= require visibility_select */ + +(() => { + const VisibilitySelect = gl.VisibilitySelect; + + describe('VisibilitySelect', function () { + const lockedElement = document.createElement('div'); + lockedElement.dataset.helpBlock = 'lockedHelpBlock'; + + const checkedElement = document.createElement('div'); + checkedElement.dataset.description = 'checkedDescription'; + + const mockElements = { + container: document.createElement('div'), + select: document.createElement('div'), + '.help-block': document.createElement('div'), + '.js-locked': lockedElement, + 'option:checked': checkedElement, + }; + + beforeEach(function () { + spyOn(Element.prototype, 'querySelector').and.callFake(selector => mockElements[selector]); + }); + + describe('#constructor', function () { + beforeEach(function () { + this.visibilitySelect = new VisibilitySelect(mockElements.container); + }); + + it('sets the container member', function () { + expect(this.visibilitySelect.container).toEqual(mockElements.container); + }); + + it('queries and sets the helpBlock member', function () { + expect(Element.prototype.querySelector).toHaveBeenCalledWith('.help-block'); + expect(this.visibilitySelect.helpBlock).toEqual(mockElements['.help-block']); + }); + + it('queries and sets the select member', function () { + expect(Element.prototype.querySelector).toHaveBeenCalledWith('select'); + expect(this.visibilitySelect.select).toEqual(mockElements.select); + }); + + describe('if there is no container element provided', function () { + it('throws an error', function () { + expect(() => new VisibilitySelect()).toThrowError('VisibilitySelect requires a container element as argument 1'); + }); + }); + }); + + describe('#init', function () { + describe('if there is a select', function () { + beforeEach(function () { + this.visibilitySelect = new VisibilitySelect(mockElements.container); + }); + + it('calls updateHelpText', function () { + spyOn(VisibilitySelect.prototype, 'updateHelpText'); + this.visibilitySelect.init(); + expect(this.visibilitySelect.updateHelpText).toHaveBeenCalled(); + }); + + it('adds a change event listener', function () { + spyOn(this.visibilitySelect.select, 'addEventListener'); + this.visibilitySelect.init(); + expect(this.visibilitySelect.select.addEventListener.calls.argsFor(0)).toContain('change'); + }); + }); + + describe('if there is no select', function () { + beforeEach(function () { + mockElements.select = undefined; + this.visibilitySelect = new VisibilitySelect(mockElements.container); + this.visibilitySelect.init(); + }); + + it('updates the helpBlock text to the locked `data-help-block` messaged', function () { + expect(this.visibilitySelect.helpBlock.textContent) + .toEqual(lockedElement.dataset.helpBlock); + }); + + afterEach(function () { + mockElements.select = document.createElement('div'); + }); + }); + }); + + describe('#updateHelpText', function () { + beforeEach(function () { + this.visibilitySelect = new VisibilitySelect(mockElements.container); + this.visibilitySelect.init(); + }); + + it('updates the helpBlock text to the selected options `data-description`', function () { + expect(this.visibilitySelect.helpBlock.textContent) + .toEqual(checkedElement.dataset.description); + }); + }); + }); +})(); -- cgit v1.2.1 From e941e96f33019fe86d84114ffb573872e12ee14a Mon Sep 17 00:00:00 2001 From: YarNayar Date: Mon, 28 Nov 2016 15:50:31 +0300 Subject: Make notification_service spec DRYer by making some tests reusable The spec for 'participatns' notifications in notification_service for both issue and merge_request is pretty simple but it was fully copied without sagnificant changes 8 times (!!!) for EVERY situation you need to notify this group of user which caused to file to extra growth and not being very DRY. And I love DRY. Since the spec is already too messy and most likely gonna to increase the size nearest time, I decided to refactor those parts. --- ...om-notification_service_spec-to-make-it-DRY.yml | 4 + spec/services/notification_service_spec.rb | 315 ++++++--------------- 2 files changed, 91 insertions(+), 228 deletions(-) create mode 100644 changelogs/unreleased/get-rid-of-water-from-notification_service_spec-to-make-it-DRY.yml diff --git a/changelogs/unreleased/get-rid-of-water-from-notification_service_spec-to-make-it-DRY.yml b/changelogs/unreleased/get-rid-of-water-from-notification_service_spec-to-make-it-DRY.yml new file mode 100644 index 00000000000..f60417d185e --- /dev/null +++ b/changelogs/unreleased/get-rid-of-water-from-notification_service_spec-to-make-it-DRY.yml @@ -0,0 +1,4 @@ +--- +title: Make notification_service spec DRYer by making test reusable +merge_request: +author: YarNayar diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index f3e80ac22a0..260e1f3fb68 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -33,6 +33,49 @@ describe NotificationService, services: true do end end + # Next shared examples are intended to test notifications of "participants" + # + # they take the following parameters: + # * issuable + # * notification trigger + # * participant + # + shared_examples 'participating by note notification' do + it 'emails the participant' do + create(:note_on_issue, noteable: issuable, project_id: project.id, note: 'anything', author: participant) + + notification_trigger + + should_email(participant) + end + end + + shared_examples 'participating by assignee notification' do + it 'emails the participant' do + issuable.update_attribute(:assignee, participant) + + notification_trigger + + should_email(participant) + end + end + + shared_examples 'participating by author notification' do + it 'emails the participant' do + issuable.author = participant + + notification_trigger + + should_email(participant) + end + end + + shared_examples_for 'participating notifications' do + it_should_behave_like 'participating by note notification' + it_should_behave_like 'participating by author notification' + it_should_behave_like 'participating by assignee notification' + end + describe 'Keys' do describe '#new_key' do let!(:key) { create(:personal_key) } @@ -539,32 +582,10 @@ describe NotificationService, services: true do should_not_email(@u_lazy_participant) end - context 'participating' do - context 'by assignee' do - before do - issue.update_attribute(:assignee, @u_lazy_participant) - notification.reassigned_issue(issue, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end - - context 'by note' do - let!(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: 'anything', author: @u_lazy_participant) } - - before { notification.reassigned_issue(issue, @u_disabled) } - - it { should_email(@u_lazy_participant) } - end - - context 'by author' do - before do - issue.author = @u_lazy_participant - notification.reassigned_issue(issue, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { issue } + let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled) } end end @@ -671,32 +692,10 @@ describe NotificationService, services: true do should_not_email(@u_lazy_participant) end - context 'participating' do - context 'by assignee' do - before do - issue.update_attribute(:assignee, @u_lazy_participant) - notification.close_issue(issue, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end - - context 'by note' do - let!(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: 'anything', author: @u_lazy_participant) } - - before { notification.close_issue(issue, @u_disabled) } - - it { should_email(@u_lazy_participant) } - end - - context 'by author' do - before do - issue.author = @u_lazy_participant - notification.close_issue(issue, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { issue } + let(:notification_trigger) { notification.close_issue(issue, @u_disabled) } end end @@ -723,32 +722,10 @@ describe NotificationService, services: true do should_not_email(@u_lazy_participant) end - context 'participating' do - context 'by assignee' do - before do - issue.update_attribute(:assignee, @u_lazy_participant) - notification.reopen_issue(issue, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end - - context 'by note' do - let!(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: 'anything', author: @u_lazy_participant) } - - before { notification.reopen_issue(issue, @u_disabled) } - - it { should_email(@u_lazy_participant) } - end - - context 'by author' do - before do - issue.author = @u_lazy_participant - notification.reopen_issue(issue, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { issue } + let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) } end end end @@ -809,31 +786,28 @@ describe NotificationService, services: true do end context 'participating' do - context 'by assignee' do - before do - merge_request.update_attribute(:assignee, @u_lazy_participant) - notification.new_merge_request(merge_request, @u_disabled) - end - - it { should_email(@u_lazy_participant) } + it_should_behave_like 'participating by assignee notification' do + let(:participant) { create(:user, username: 'user-participant')} + let(:issuable) { merge_request } + let(:notification_trigger) { notification.new_merge_request(merge_request, @u_disabled) } end - context 'by note' do - let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) } - - before { notification.new_merge_request(merge_request, @u_disabled) } - - it { should_email(@u_lazy_participant) } + it_should_behave_like 'participating by note notification' do + let(:participant) { create(:user, username: 'user-participant')} + let(:issuable) { merge_request } + let(:notification_trigger) { notification.new_merge_request(merge_request, @u_disabled) } end context 'by author' do + let(:participant) { create(:user, username: 'user-participant')} + before do - merge_request.author = @u_lazy_participant + merge_request.author = participant merge_request.save notification.new_merge_request(merge_request, @u_disabled) end - it { should_not_email(@u_lazy_participant) } + it { should_not_email(participant) } end end end @@ -868,33 +842,10 @@ describe NotificationService, services: true do should_not_email(@u_lazy_participant) end - context 'participating' do - context 'by assignee' do - before do - merge_request.update_attribute(:assignee, @u_lazy_participant) - notification.reassigned_merge_request(merge_request, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end - - context 'by note' do - let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) } - - before { notification.reassigned_merge_request(merge_request, @u_disabled) } - - it { should_email(@u_lazy_participant) } - end - - context 'by author' do - before do - merge_request.author = @u_lazy_participant - merge_request.save - notification.reassigned_merge_request(merge_request, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { merge_request } + let(:notification_trigger) { notification.reassigned_merge_request(merge_request, @u_disabled) } end end @@ -965,33 +916,10 @@ describe NotificationService, services: true do should_not_email(@u_lazy_participant) end - context 'participating' do - context 'by assignee' do - before do - merge_request.update_attribute(:assignee, @u_lazy_participant) - notification.close_mr(merge_request, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end - - context 'by note' do - let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) } - - before { notification.close_mr(merge_request, @u_disabled) } - - it { should_email(@u_lazy_participant) } - end - - context 'by author' do - before do - merge_request.author = @u_lazy_participant - merge_request.save - notification.close_mr(merge_request, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { merge_request } + let(:notification_trigger) { notification.close_mr(merge_request, @u_disabled) } end end @@ -1032,33 +960,10 @@ describe NotificationService, services: true do should_not_email(@u_watcher) end - context 'participating' do - context 'by assignee' do - before do - merge_request.update_attribute(:assignee, @u_lazy_participant) - notification.merge_mr(merge_request, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end - - context 'by note' do - let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) } - - before { notification.merge_mr(merge_request, @u_disabled) } - - it { should_email(@u_lazy_participant) } - end - - context 'by author' do - before do - merge_request.author = @u_lazy_participant - merge_request.save - notification.merge_mr(merge_request, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { merge_request } + let(:notification_trigger) { notification.merge_mr(merge_request, @u_disabled) } end end @@ -1085,33 +990,10 @@ describe NotificationService, services: true do should_not_email(@u_lazy_participant) end - context 'participating' do - context 'by assignee' do - before do - merge_request.update_attribute(:assignee, @u_lazy_participant) - notification.reopen_mr(merge_request, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end - - context 'by note' do - let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) } - - before { notification.reopen_mr(merge_request, @u_disabled) } - - it { should_email(@u_lazy_participant) } - end - - context 'by author' do - before do - merge_request.author = @u_lazy_participant - merge_request.save - notification.reopen_mr(merge_request, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { merge_request } + let(:notification_trigger) { notification.reopen_mr(merge_request, @u_disabled) } end end @@ -1131,33 +1013,10 @@ describe NotificationService, services: true do should_not_email(@u_lazy_participant) end - context 'participating' do - context 'by assignee' do - before do - merge_request.update_attribute(:assignee, @u_lazy_participant) - notification.resolve_all_discussions(merge_request, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end - - context 'by note' do - let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) } - - before { notification.resolve_all_discussions(merge_request, @u_disabled) } - - it { should_email(@u_lazy_participant) } - end - - context 'by author' do - before do - merge_request.author = @u_lazy_participant - merge_request.save - notification.resolve_all_discussions(merge_request, @u_disabled) - end - - it { should_email(@u_lazy_participant) } - end + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { merge_request } + let(:notification_trigger) { notification.resolve_all_discussions(merge_request, @u_disabled) } end end end -- cgit v1.2.1 From afea2df15121c0fa4d36f16b050421f681564b64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Wed, 14 Dec 2016 12:14:25 +0100 Subject: First attempt --- lib/ci/api/builds.rb | 17 +++++++++++++++++ lib/ci/api/helpers.rb | 3 +++ 2 files changed, 20 insertions(+) diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index ed87a2603e8..1ec4584a13f 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -16,6 +16,15 @@ module Ci not_found! unless current_runner.active? update_runner_info + last_update = Gitlab::Redis.with { |redis| redis.get(current_runner_redis_key)} + + if params[:last_update] != "" + if :last_update == last_update + headers 'X-GitLab-Last-Update', last_update + return build_not_found! + end + end + build = Ci::RegisterBuildService.new.execute(current_runner) if build @@ -26,6 +35,14 @@ module Ci else Gitlab::Metrics.add_event(:build_not_found) + if last_update == "" + Gitlab::Redis.with do |redis] + new_update = Time.new.inspect + redis.set(current_runner_redis_key, new_update, ex: 60.minutes) + headers 'X-GitLab-Last-Update', new_update + end + end + build_not_found! end end diff --git a/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb index e608f5f6cad..ef80e0df5d3 100644 --- a/lib/ci/api/helpers.rb +++ b/lib/ci/api/helpers.rb @@ -60,6 +60,9 @@ module Ci @runner ||= Runner.find_by_token(params[:token].to_s) end + def current_runner_redis_key + @runner_redis_key ||= "#{current_runner.token}_#{current_runner.tag_list}" + def get_runner_version_from_params return unless params["info"].present? attributes_for_keys(["name", "version", "revision", "platform", "architecture"], params["info"]) -- cgit v1.2.1 From a6ab8a34093ccd2de7286797ccc639c1cbf267fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Wed, 14 Dec 2016 16:06:50 +0100 Subject: Add BuildQueueWorker for injecting redis-keys --- app/models/ci/build.rb | 6 ++++++ app/workers/build_queue_worker.rb | 18 ++++++++++++++++++ lib/ci/api/helpers.rb | 1 + 3 files changed, 25 insertions(+) create mode 100644 app/workers/build_queue_worker.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 88c46076df6..cab9393344b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -81,6 +81,12 @@ module Ci end state_machine :status do + after_transition any => [:pending] do |build| + build.run_after_commit do + BuildQueueWorker.perform_async(id) + end + end + after_transition pending: :running do |build| build.run_after_commit do BuildHooksWorker.perform_async(id) diff --git a/app/workers/build_queue_worker.rb b/app/workers/build_queue_worker.rb new file mode 100644 index 00000000000..f19290b0c1a --- /dev/null +++ b/app/workers/build_queue_worker.rb @@ -0,0 +1,18 @@ +class BuildQueueWorker + include Sidekiq::Worker + include BuildQueue + + def perform(build_id) + Ci::Build.find_by(id: build_id).try do |build| + project.runners.select do |runner| + if runner.can_pick?(build) + # Inject last_update into Redis + Gitlab::Redis.with do |redis] + new_update = Time.new.inspect + redis.set(current_runner_redis_key, new_update, ex: 60.minutes) + end + end + end + end + end +end diff --git a/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb index ef80e0df5d3..74e1871619e 100644 --- a/lib/ci/api/helpers.rb +++ b/lib/ci/api/helpers.rb @@ -62,6 +62,7 @@ module Ci def current_runner_redis_key @runner_redis_key ||= "#{current_runner.token}_#{current_runner.tag_list}" + end def get_runner_version_from_params return unless params["info"].present? -- cgit v1.2.1 From 0d43815620f96a061d7bc5c2c53748cac879c518 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Wed, 14 Dec 2016 16:33:16 +0100 Subject: This is needed as well... --- app/workers/build_queue_worker.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/workers/build_queue_worker.rb b/app/workers/build_queue_worker.rb index f19290b0c1a..1ec020039cf 100644 --- a/app/workers/build_queue_worker.rb +++ b/app/workers/build_queue_worker.rb @@ -9,10 +9,17 @@ class BuildQueueWorker # Inject last_update into Redis Gitlab::Redis.with do |redis] new_update = Time.new.inspect - redis.set(current_runner_redis_key, new_update, ex: 60.minutes) + redis.set(runner_redis_key(runner), new_update, ex: 60.minutes) end end end end end + + private + + def runner_redis_key(runner) + "#{runner.token}_#{runner.tag_list}" + end + end -- cgit v1.2.1 From aa224c13fc8d9712e634dcb3843c73d3838fb20a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Wed, 14 Dec 2016 16:39:40 +0100 Subject: typo-o --- app/workers/build_queue_worker.rb | 2 +- lib/ci/api/builds.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/workers/build_queue_worker.rb b/app/workers/build_queue_worker.rb index 1ec020039cf..4cb910e90ca 100644 --- a/app/workers/build_queue_worker.rb +++ b/app/workers/build_queue_worker.rb @@ -7,7 +7,7 @@ class BuildQueueWorker project.runners.select do |runner| if runner.can_pick?(build) # Inject last_update into Redis - Gitlab::Redis.with do |redis] + Gitlab::Redis.with do |redis| new_update = Time.new.inspect redis.set(runner_redis_key(runner), new_update, ex: 60.minutes) end diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index 1ec4584a13f..acb0ca8719f 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -36,7 +36,7 @@ module Ci Gitlab::Metrics.add_event(:build_not_found) if last_update == "" - Gitlab::Redis.with do |redis] + Gitlab::Redis.with do |redis| new_update = Time.new.inspect redis.set(current_runner_redis_key, new_update, ex: 60.minutes) headers 'X-GitLab-Last-Update', new_update -- cgit v1.2.1 From 692426b13ecff871890a853bebef19d63ee4985e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Wed, 14 Dec 2016 17:05:09 +0100 Subject: Use correct variables --- app/workers/build_queue_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/build_queue_worker.rb b/app/workers/build_queue_worker.rb index 4cb910e90ca..141c1543aeb 100644 --- a/app/workers/build_queue_worker.rb +++ b/app/workers/build_queue_worker.rb @@ -4,7 +4,7 @@ class BuildQueueWorker def perform(build_id) Ci::Build.find_by(id: build_id).try do |build| - project.runners.select do |runner| + build.project.runners.select do |runner| if runner.can_pick?(build) # Inject last_update into Redis Gitlab::Redis.with do |redis| -- cgit v1.2.1 From 453be7c03aef2ba9c127199a64146ba483c43448 Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Thu, 15 Dec 2016 12:28:53 +0000 Subject: Added recommendation to install github-markdown --- app/views/projects/wikis/git_access.html.haml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/views/projects/wikis/git_access.html.haml b/app/views/projects/wikis/git_access.html.haml index e25d6a48573..a1db57739f4 100644 --- a/app/views/projects/wikis/git_access.html.haml +++ b/app/views/projects/wikis/git_access.html.haml @@ -14,6 +14,10 @@ .wiki-git-access %h3 Install Gollum + %p= It is recommended to install github-markdown so that GFM features render locally + %pre.dark + :preserve + gem install github-markdown %pre.dark :preserve gem install gollum -- cgit v1.2.1 From 771dd68cdff9c1f57082fd99af5c22c3af96e7c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Thu, 15 Dec 2016 22:29:47 +0100 Subject: linting --- app/models/ci/build.rb | 2 +- lib/ci/api/builds.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index cab9393344b..5c84833b806 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -83,7 +83,7 @@ module Ci state_machine :status do after_transition any => [:pending] do |build| build.run_after_commit do - BuildQueueWorker.perform_async(id) + BuildQueueWorker.perform_async(id) end end diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index acb0ca8719f..df32f64ef83 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -18,8 +18,8 @@ module Ci last_update = Gitlab::Redis.with { |redis| redis.get(current_runner_redis_key)} - if params[:last_update] != "" - if :last_update == last_update + if params[:last_update].present? + if params[:last_update] == last_update headers 'X-GitLab-Last-Update', last_update return build_not_found! end -- cgit v1.2.1 From e179544342247c9a227c1ef3c3e709a0449158e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Thu, 15 Dec 2016 22:30:31 +0100 Subject: Move redis-logic into Ci::Runner --- app/models/ci/runner.rb | 16 ++++++++++++++++ lib/ci/api/builds.rb | 11 +++-------- lib/ci/api/helpers.rb | 4 ---- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 123930273e0..30f5a2ee092 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -21,6 +21,8 @@ module Ci scope :online, ->() { where('contacted_at > ?', LAST_CONTACT_TIME) } scope :ordered, ->() { order(id: :desc) } + after_save :tick_update + scope :owned_or_shared, ->(project_id) do joins('LEFT JOIN ci_runner_projects ON ci_runner_projects.runner_id = ci_runners.id') .where("ci_runner_projects.gl_project_id = :project_id OR ci_runners.is_shared = true", project_id: project_id) @@ -122,8 +124,22 @@ module Ci ] end + def tick_update + new_update = Time.new.inspect + Gitlab::Redis.with { |redis| redis.set(redis_key, new_update, ex: 60.minutes) } + new_update + end + + def last_build_queue_update + Gitlab::Redis.with { |redis| redis.get(redis_key) } + end + private + def redis_key + "runner:build_queue:#{self.id}" + end + def tag_constraints unless has_tags? || run_untagged? errors.add(:tags_list, diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index df32f64ef83..fb14f043a18 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -16,7 +16,7 @@ module Ci not_found! unless current_runner.active? update_runner_info - last_update = Gitlab::Redis.with { |redis| redis.get(current_runner_redis_key)} + last_update = current_runner.last_build_queue_update if params[:last_update].present? if params[:last_update] == last_update @@ -35,13 +35,8 @@ module Ci else Gitlab::Metrics.add_event(:build_not_found) - if last_update == "" - Gitlab::Redis.with do |redis| - new_update = Time.new.inspect - redis.set(current_runner_redis_key, new_update, ex: 60.minutes) - headers 'X-GitLab-Last-Update', new_update - end - end + new_update = current_runner.tick_update + headers 'X-GitLab-Last-Update', new_update build_not_found! end diff --git a/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb index 74e1871619e..e608f5f6cad 100644 --- a/lib/ci/api/helpers.rb +++ b/lib/ci/api/helpers.rb @@ -60,10 +60,6 @@ module Ci @runner ||= Runner.find_by_token(params[:token].to_s) end - def current_runner_redis_key - @runner_redis_key ||= "#{current_runner.token}_#{current_runner.tag_list}" - end - def get_runner_version_from_params return unless params["info"].present? attributes_for_keys(["name", "version", "revision", "platform", "architecture"], params["info"]) -- cgit v1.2.1 From a20681c093d5444d24c54f41fd49a98ef919cd42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Thu, 15 Dec 2016 22:37:52 +0100 Subject: Update Runners in a Service --- app/services/ci/update_build_queue_service.rb | 12 ++++++++++++ app/workers/build_queue_worker.rb | 16 +--------------- 2 files changed, 13 insertions(+), 15 deletions(-) create mode 100644 app/services/ci/update_build_queue_service.rb diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb new file mode 100644 index 00000000000..348ba6f5cc4 --- /dev/null +++ b/app/services/ci/update_build_queue_service.rb @@ -0,0 +1,12 @@ +module Ci + class UpdateBuildQueueService < BaseService + + def execute(build) + build.project.runners.select do |runner| + if runner.can_pick?(build) + runner.tick_update + end + end + end + end +end diff --git a/app/workers/build_queue_worker.rb b/app/workers/build_queue_worker.rb index 141c1543aeb..8cc99eabdfe 100644 --- a/app/workers/build_queue_worker.rb +++ b/app/workers/build_queue_worker.rb @@ -4,22 +4,8 @@ class BuildQueueWorker def perform(build_id) Ci::Build.find_by(id: build_id).try do |build| - build.project.runners.select do |runner| - if runner.can_pick?(build) - # Inject last_update into Redis - Gitlab::Redis.with do |redis| - new_update = Time.new.inspect - redis.set(runner_redis_key(runner), new_update, ex: 60.minutes) - end - end - end + UpdateBuildQueueService.new(build) end end - private - - def runner_redis_key(runner) - "#{runner.token}_#{runner.tag_list}" - end - end -- cgit v1.2.1 From 811addf2d16bd912efa095b6b5a5f93d620089ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Fri, 16 Dec 2016 01:04:18 +0100 Subject: #NamingThings --- app/models/ci/runner.rb | 21 ++++++++++++++++----- app/services/ci/update_build_queue_service.rb | 1 - app/workers/build_queue_worker.rb | 3 +-- lib/ci/api/builds.rb | 12 ++++-------- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 30f5a2ee092..4f96f4f3645 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -124,19 +124,30 @@ module Ci ] end - def tick_update + def tick_runner_queue new_update = Time.new.inspect - Gitlab::Redis.with { |redis| redis.set(redis_key, new_update, ex: 60.minutes) } + Gitlab::Redis.with { |redis| redis.set(runner_queue_key, new_update, ex: 60.minutes) } new_update end - def last_build_queue_update - Gitlab::Redis.with { |redis| redis.get(redis_key) } + def ensure_runner_queue_value + Gitlab::Redis.with do |redis| + value = redis.get(runner_queue_key) + if value == "" + value = Time.new.inspect + redis.set(runner_queue_key, value, ex: 60.minutes) + end + value + end + end + + def is_runner_queue_value_latest?(value) + ensure_runner_queue_value == value if value.present? end private - def redis_key + def runner_queue_key "runner:build_queue:#{self.id}" end diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb index 348ba6f5cc4..dd1097f5dbe 100644 --- a/app/services/ci/update_build_queue_service.rb +++ b/app/services/ci/update_build_queue_service.rb @@ -1,6 +1,5 @@ module Ci class UpdateBuildQueueService < BaseService - def execute(build) build.project.runners.select do |runner| if runner.can_pick?(build) diff --git a/app/workers/build_queue_worker.rb b/app/workers/build_queue_worker.rb index 8cc99eabdfe..c0ceb8d423a 100644 --- a/app/workers/build_queue_worker.rb +++ b/app/workers/build_queue_worker.rb @@ -4,8 +4,7 @@ class BuildQueueWorker def perform(build_id) Ci::Build.find_by(id: build_id).try do |build| - UpdateBuildQueueService.new(build) + UpdateBuildQueueService.execute(build) end end - end diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index fb14f043a18..b1b66313092 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -16,13 +16,9 @@ module Ci not_found! unless current_runner.active? update_runner_info - last_update = current_runner.last_build_queue_update - - if params[:last_update].present? - if params[:last_update] == last_update - headers 'X-GitLab-Last-Update', last_update - return build_not_found! - end + if current_runner.is_runner_queue_value_latest?(params[:last_update]) + headers 'X-GitLab-Last-Update', params[:last_update] + return build_not_found! end build = Ci::RegisterBuildService.new.execute(current_runner) @@ -35,7 +31,7 @@ module Ci else Gitlab::Metrics.add_event(:build_not_found) - new_update = current_runner.tick_update + new_update = current_runner.ensure_runner_queue_value headers 'X-GitLab-Last-Update', new_update build_not_found! -- cgit v1.2.1 From 94b2df022449352953a2d5e607de1a3d31e88b47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Fri, 16 Dec 2016 01:20:05 +0100 Subject: Make ensure_runner_queue_value atomic --- app/models/ci/runner.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 4f96f4f3645..a26a67739a1 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -132,12 +132,14 @@ module Ci def ensure_runner_queue_value Gitlab::Redis.with do |redis| - value = redis.get(runner_queue_key) - if value == "" - value = Time.new.inspect - redis.set(runner_queue_key, value, ex: 60.minutes) + redis.multi do + value = redis.get(runner_queue_key) + if value == "" + value = Time.new.inspect + redis.set(runner_queue_key, value, ex: 60.minutes) + end + value end - value end end -- cgit v1.2.1 From f6263e2ee7288ff1b53b551053911f31ddf846b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Fri, 16 Dec 2016 16:29:11 +0100 Subject: Don't use redis.multi --- app/models/ci/runner.rb | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index a26a67739a1..e880ea9a880 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -21,7 +21,7 @@ module Ci scope :online, ->() { where('contacted_at > ?', LAST_CONTACT_TIME) } scope :ordered, ->() { order(id: :desc) } - after_save :tick_update + after_save :tick_runner_queue scope :owned_or_shared, ->(project_id) do joins('LEFT JOIN ci_runner_projects ON ci_runner_projects.runner_id = ci_runners.id') @@ -132,14 +132,8 @@ module Ci def ensure_runner_queue_value Gitlab::Redis.with do |redis| - redis.multi do - value = redis.get(runner_queue_key) - if value == "" - value = Time.new.inspect - redis.set(runner_queue_key, value, ex: 60.minutes) - end - value - end + redis.set(runner_queue_key, value, ex: 60.minutes, nx: true) + redis.get(runner_queue_key) end end -- cgit v1.2.1 From 24f0a45b0f8558071987ec1d70446f8e9f7df0ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Fri, 16 Dec 2016 16:32:16 +0100 Subject: Do things in the correct order --- app/models/ci/runner.rb | 1 + lib/ci/api/builds.rb | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index e880ea9a880..221da8adf32 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -132,6 +132,7 @@ module Ci def ensure_runner_queue_value Gitlab::Redis.with do |redis| + value = Time.new.inspect redis.set(runner_queue_key, value, ex: 60.minutes, nx: true) redis.get(runner_queue_key) end diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index b1b66313092..8264210c460 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -21,6 +21,8 @@ module Ci return build_not_found! end + new_update = current_runner.ensure_runner_queue_value + build = Ci::RegisterBuildService.new.execute(current_runner) if build @@ -31,7 +33,6 @@ module Ci else Gitlab::Metrics.add_event(:build_not_found) - new_update = current_runner.ensure_runner_queue_value headers 'X-GitLab-Last-Update', new_update build_not_found! -- cgit v1.2.1 From fb713071db822cd5946e201d749ebee42ecbae0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Fri, 16 Dec 2016 16:33:08 +0100 Subject: Use Token instead of ID --- app/models/ci/runner.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 221da8adf32..d5b5a84d477 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -145,7 +145,7 @@ module Ci private def runner_queue_key - "runner:build_queue:#{self.id}" + "runner:build_queue:#{self.token}" end def tag_constraints -- cgit v1.2.1 From b0b05f811cc11472798661de713e2564cae2e9d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Fri, 16 Dec 2016 16:39:40 +0100 Subject: Make expire-time a constant, correct function in Service --- app/models/ci/runner.rb | 5 +++-- app/services/ci/update_build_queue_service.rb | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index d5b5a84d477..850be67d98f 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -2,6 +2,7 @@ module Ci class Runner < ActiveRecord::Base extend Ci::Model + LAST_UPDATE_EXPIRE_TIME = 60.minutes LAST_CONTACT_TIME = 1.hour.ago AVAILABLE_SCOPES = %w[specific shared active paused online] FORM_EDITABLE = %i[description tag_list active run_untagged locked] @@ -126,14 +127,14 @@ module Ci def tick_runner_queue new_update = Time.new.inspect - Gitlab::Redis.with { |redis| redis.set(runner_queue_key, new_update, ex: 60.minutes) } + Gitlab::Redis.with { |redis| redis.set(runner_queue_key, new_update, ex: LAST_UPDATE_EXPIRE_TIME) } new_update end def ensure_runner_queue_value Gitlab::Redis.with do |redis| value = Time.new.inspect - redis.set(runner_queue_key, value, ex: 60.minutes, nx: true) + redis.set(runner_queue_key, value, ex: LAST_UPDATE_EXPIRE_TIME, nx: true) redis.get(runner_queue_key) end end diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb index dd1097f5dbe..b324863feef 100644 --- a/app/services/ci/update_build_queue_service.rb +++ b/app/services/ci/update_build_queue_service.rb @@ -3,7 +3,7 @@ module Ci def execute(build) build.project.runners.select do |runner| if runner.can_pick?(build) - runner.tick_update + runner.tick_runner_queue end end end -- cgit v1.2.1 From 74a48ecd6bce154dff9704575ba3d0824a73e8e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Fri, 16 Dec 2016 17:42:35 +0100 Subject: Change name of expire constant --- app/models/ci/runner.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 850be67d98f..d13e54fcc16 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -2,7 +2,7 @@ module Ci class Runner < ActiveRecord::Base extend Ci::Model - LAST_UPDATE_EXPIRE_TIME = 60.minutes + RUNNER_QUEUE_EXPIRY_TIME = 60.minutes LAST_CONTACT_TIME = 1.hour.ago AVAILABLE_SCOPES = %w[specific shared active paused online] FORM_EDITABLE = %i[description tag_list active run_untagged locked] @@ -127,14 +127,14 @@ module Ci def tick_runner_queue new_update = Time.new.inspect - Gitlab::Redis.with { |redis| redis.set(runner_queue_key, new_update, ex: LAST_UPDATE_EXPIRE_TIME) } + Gitlab::Redis.with { |redis| redis.set(runner_queue_key, new_update, ex: RUNNER_QUEUE_EXPIRY_TIME) } new_update end def ensure_runner_queue_value Gitlab::Redis.with do |redis| value = Time.new.inspect - redis.set(runner_queue_key, value, ex: LAST_UPDATE_EXPIRE_TIME, nx: true) + redis.set(runner_queue_key, value, ex: RUNNER_QUEUE_EXPIRY_TIME, nx: true) redis.get(runner_queue_key) end end -- cgit v1.2.1 From f14228f0f2f69a967c483e4aa4ef1568e5fdc49b Mon Sep 17 00:00:00 2001 From: twonegatives Date: Tue, 13 Dec 2016 00:55:33 +0300 Subject: Notify the user who set auto-merge when a build fails --- app/helpers/todos_helper.rb | 2 +- app/services/todo_service.rb | 15 +++++++++------ .../23524-notify-automerge-user-of-failed-build.yml | 4 ++++ spec/services/todo_service_spec.rb | 7 +++++++ 4 files changed, 21 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index 09c69786791..3c039b43f5d 100644 --- a/app/helpers/todos_helper.rb +++ b/app/helpers/todos_helper.rb @@ -11,7 +11,7 @@ module TodosHelper case todo.action when Todo::ASSIGNED then 'assigned you' when Todo::MENTIONED then 'mentioned you on' - when Todo::BUILD_FAILED then 'The build failed for your' + when Todo::BUILD_FAILED then 'The build failed for' when Todo::MARKED then 'added a todo for' when Todo::APPROVAL_REQUIRED then 'set you as an approver for' end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index f8e6b2ef094..d67c166275a 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -98,10 +98,12 @@ class TodoService # When a build fails on the HEAD of a merge request we should: # - # * create a todo for that user to fix it + # * create a todo for author of MR to fix it + # * create a todo for merge_user to keep an eye on it # def merge_request_build_failed(merge_request) - create_build_failed_todo(merge_request) + create_build_failed_todo(merge_request, merge_request.author) + create_build_failed_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds? end # When a new commit is pushed to a merge request we should: @@ -115,9 +117,11 @@ class TodoService # When a build is retried to a merge request we should: # # * mark all pending todos related to the merge request for the author as done + # * mark all pending todos related to the merge request for the merge_user as done # def merge_request_build_retried(merge_request) mark_pending_todos_as_done(merge_request, merge_request.author) + mark_pending_todos_as_done(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds? end # When create a note we should: @@ -236,10 +240,9 @@ class TodoService create_todos(mentioned_users, attributes) end - def create_build_failed_todo(merge_request) - author = merge_request.author - attributes = attributes_for_todo(merge_request.project, merge_request, author, Todo::BUILD_FAILED) - create_todos(author, attributes) + def create_build_failed_todo(merge_request, todo_author) + attributes = attributes_for_todo(merge_request.project, merge_request, todo_author, Todo::BUILD_FAILED) + create_todos(todo_author, attributes) end def attributes_for_target(target) diff --git a/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml b/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml new file mode 100644 index 00000000000..4b6f9d1efe9 --- /dev/null +++ b/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml @@ -0,0 +1,4 @@ +--- +title: Create a TODO for user who set auto-merge when a build fails +merge_request: +author: diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index ed55791d24e..d8a9ca20b36 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -469,6 +469,13 @@ describe TodoService, services: true do should_create_todo(user: author, target: mr_unassigned, action: Todo::BUILD_FAILED) end + + it 'creates a pending todo for merge_user' do + mr_unassigned.update(merge_when_build_succeeds: true, merge_user: admin) + service.merge_request_build_failed(mr_unassigned) + + should_create_todo(user: admin, author: admin, target: mr_unassigned, action: Todo::BUILD_FAILED) + end end describe '#merge_request_push' do -- cgit v1.2.1 From f35336a1e6b1eb750a501a5d54396816f4800e69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Wed, 21 Dec 2016 04:27:03 +0100 Subject: Fixed broken build --- app/services/ci/update_build_queue_service.rb | 2 +- app/workers/build_queue_worker.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb index b324863feef..704b872ddd0 100644 --- a/app/services/ci/update_build_queue_service.rb +++ b/app/services/ci/update_build_queue_service.rb @@ -1,5 +1,5 @@ module Ci - class UpdateBuildQueueService < BaseService + class UpdateBuildQueueService def execute(build) build.project.runners.select do |runner| if runner.can_pick?(build) diff --git a/app/workers/build_queue_worker.rb b/app/workers/build_queue_worker.rb index c0ceb8d423a..fa9e097e40a 100644 --- a/app/workers/build_queue_worker.rb +++ b/app/workers/build_queue_worker.rb @@ -4,7 +4,7 @@ class BuildQueueWorker def perform(build_id) Ci::Build.find_by(id: build_id).try do |build| - UpdateBuildQueueService.execute(build) + Ci::UpdateBuildQueueService.new.execute(build) end end end -- cgit v1.2.1 From 2c9ee1bb42255be818e6e13be19cb7897f2ca7fd Mon Sep 17 00:00:00 2001 From: Bryce Johnson Date: Wed, 21 Dec 2016 10:58:00 +0000 Subject: Add documentation for possible causes of JS-related test failures. --- doc/development/frontend.md | 78 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 68 insertions(+), 10 deletions(-) diff --git a/doc/development/frontend.md b/doc/development/frontend.md index 9e782ab977f..5922f15adac 100644 --- a/doc/development/frontend.md +++ b/doc/development/frontend.md @@ -297,16 +297,74 @@ For our currently-supported browsers, see our [requirements][requirements]. ## Gotchas -### Phantom.JS (used by Teaspoon & Rspec) chokes, returning vague JavaScript errors - -If you see very generic JavaScript errors (e.g. `jQuery is undefined`) being thrown in tests, but -can't reproduce them manually, you may have included `ES6`-style JavaScript in files that don't -have the `.js.es6` file extension. Either use ES5-friendly JavaScript or rename the file you're -working in (`git mv `). - -Similar errors will be thrown if you're using -any of the [array methods introduced in ES6](http://www.2ality.com/2014/05/es6-array-methods.html) -whether or not you've updated the file extension. +### Spec errors due to use of ES6 features in `.js` files + +If you see very generic JavaScript errors (e.g. `jQuery is undefined`) being +thrown in Teaspoon, Spinach, or Rspec tests but can't reproduce them manually, +you may have included `ES6`-style JavaScript in files that don't have the +`.js.es6` file extension. Either use ES5-friendly JavaScript or rename the file +you're working in (`git mv `). + +### Spec errors due to use of unsupported JavaScript + +Similar errors will be thrown if you're using JavaScript features not yet +supported by our test runner's version of webkit, whether or not you've updated +the file extension. Examples of unsupported JavaScript features are: + +- Array.from +- Array.find +- Array.first +- Object.assign +- Async functions +- Generators +- Array destructuring +- For Of +- Symbol/Symbol.iterator] +- Spread + +Until these are polyfilled or transpiled appropriately, they should not be used. +Please update this list with additional unsupported features or when any of +these are made usable. + +### Spec errors due to JavaScript not enabled + +If, as a result of a change you've made, a feature now depends on JavaScript to +run correctly, you need to make sure a JavaScript web driver is enabled when +specs are run. If you don't you'll see vague error messages from the spec +runner, and an explosion of vague console errors in the HTML snapshot. + +To enable a JavaScript driver in an `rspec` test, add `js: true` to the +individual spec or the context block containing multiple specs that need +JavaScript enabled: + +```ruby + +# For one spec +it 'presents information about abuse report', js: true do + # assertions... +end + +describe "Admin::AbuseReports", js: true do + it 'presents information about abuse report' do + # assertions... + end + it 'shows buttons for adding to abuse report' do + # assertions... + end +end +``` +In Spinach, the JavaScript driver is enabled differently. In the `*.feature` +file for the failing spec, add the @javascript flag above the Scenario: +``` +@javascript +Scenario: Developer can approve merge request + Given I am a "Shop" developer + And I visit project "Shop" merge requests page + And merge request 'Bug NS-04' must be approved + And I click link "Bug NS-04" + When I click link "Approve" + Then I should see approved merge request "Bug NS-04" +``` -- cgit v1.2.1 From ec5c12ce77b5e354e2fd43d6a46438af6c3fb098 Mon Sep 17 00:00:00 2001 From: Bryce Johnson Date: Wed, 21 Dec 2016 15:11:22 +0000 Subject: Fix markdown errors. --- doc/development/frontend.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/development/frontend.md b/doc/development/frontend.md index 5922f15adac..f79bd23dc90 100644 --- a/doc/development/frontend.md +++ b/doc/development/frontend.md @@ -319,7 +319,7 @@ the file extension. Examples of unsupported JavaScript features are: - Generators - Array destructuring - For Of -- Symbol/Symbol.iterator] +- Symbol/Symbol.iterator - Spread Until these are polyfilled or transpiled appropriately, they should not be used. @@ -355,7 +355,7 @@ end ``` In Spinach, the JavaScript driver is enabled differently. In the `*.feature` -file for the failing spec, add the @javascript flag above the Scenario: +file for the failing spec, add the `@javascript` flag above the Scenario: ``` @javascript -- cgit v1.2.1 From 95047f1dc7109ea7e5ebead4115ec01bb8a75ba3 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Fri, 30 Dec 2016 17:30:01 +0200 Subject: Make diff discussion more reliable --- app/models/merge_request.rb | 8 +++++--- changelogs/unreleased/fix_broken_diff_discussions.yml | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/fix_broken_diff_discussions.yml diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 61845bf4036..83f752d6826 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -861,9 +861,11 @@ class MergeRequest < ActiveRecord::Base paths: paths ) - active_diff_notes.each do |note| - service.execute(note) - Gitlab::Timeless.timeless(note, &:save) + transaction do + active_diff_notes.each do |note| + service.execute(note) + Gitlab::Timeless.timeless(note, &:save) + end end end diff --git a/changelogs/unreleased/fix_broken_diff_discussions.yml b/changelogs/unreleased/fix_broken_diff_discussions.yml new file mode 100644 index 00000000000..4551212759f --- /dev/null +++ b/changelogs/unreleased/fix_broken_diff_discussions.yml @@ -0,0 +1,4 @@ +--- +title: Make MR-review-discussions more reliable +merge_request: +author: -- cgit v1.2.1 From 52867e15acacf842e26816c9143c59fc9086c6fb Mon Sep 17 00:00:00 2001 From: Brian Neel Date: Mon, 14 Nov 2016 20:30:12 -0500 Subject: Disable automatic login feature when clicking on email confirmation links --- app/controllers/confirmations_controller.rb | 8 ++------ .../unreleased/disable-autologin-on-email-confirmation-links.yml | 4 ++++ 2 files changed, 6 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/disable-autologin-on-email-confirmation-links.yml diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb index 3da44b9b888..306afb65f10 100644 --- a/app/controllers/confirmations_controller.rb +++ b/app/controllers/confirmations_controller.rb @@ -14,12 +14,8 @@ class ConfirmationsController < Devise::ConfirmationsController if signed_in?(resource_name) after_sign_in_path_for(resource) else - sign_in(resource) - if signed_in?(resource_name) - after_sign_in_path_for(resource) - else - new_session_path(resource_name) - end + flash[:notice] += " Please sign in." + new_session_path(resource_name) end end end diff --git a/changelogs/unreleased/disable-autologin-on-email-confirmation-links.yml b/changelogs/unreleased/disable-autologin-on-email-confirmation-links.yml new file mode 100644 index 00000000000..6dd0d748001 --- /dev/null +++ b/changelogs/unreleased/disable-autologin-on-email-confirmation-links.yml @@ -0,0 +1,4 @@ +--- +title: Disable automatic login after clicking email confirmation links +merge_request: 7472 +author: -- cgit v1.2.1 From b46ad4c90b779102af8545a0d9a0789a9fc4c085 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 27 Dec 2016 10:32:50 +0100 Subject: Fix Slash command templates These templates were broken, as the same views didn't have a @project defined. Now I've added checks on the service being a template or not. Fixes #26042 --- .../mattermost_slash_commands/_help.html.haml | 4 +- .../services/slack_slash_commands/_help.html.haml | 144 +++++++++++---------- 2 files changed, 75 insertions(+), 73 deletions(-) diff --git a/app/views/projects/services/mattermost_slash_commands/_help.html.haml b/app/views/projects/services/mattermost_slash_commands/_help.html.haml index 63b797cd391..c1e576b42fc 100644 --- a/app/views/projects/services/mattermost_slash_commands/_help.html.haml +++ b/app/views/projects/services/mattermost_slash_commands/_help.html.haml @@ -8,8 +8,8 @@ by entering %code /<command_trigger_word> help - - unless enabled + - unless enabled || @service.template? = render 'projects/services/mattermost_slash_commands/detailed_help', subject: @service -- if enabled +- if enabled && !@service.template? = render 'projects/services/mattermost_slash_commands/installation_info', subject: @service diff --git a/app/views/projects/services/slack_slash_commands/_help.html.haml b/app/views/projects/services/slack_slash_commands/_help.html.haml index 6d7c2defe2b..8aac1b0c7da 100644 --- a/app/views/projects/services/slack_slash_commands/_help.html.haml +++ b/app/views/projects/services/slack_slash_commands/_help.html.haml @@ -1,4 +1,5 @@ -- run_actions_text = "Perform common operations on this project: #{@project.name_with_namespace}" +- pretty_name = defined?(@project) ? @project.path_with_namespace : "namespace / path" +- run_actions_text = "Perform common operations on this project: #{pretty_name}" .well This service allows GitLab users to perform common operations on this @@ -9,85 +10,86 @@ %code /<command> help %br %br - To setup this service: - %ul.list-unstyled - %li - 1. - = link_to 'Add a slash command', 'https://my.slack.com/services/new/slash-commands' - in your Slack team with these options: + - unless @service.template? + To setup this service: + %ul.list-unstyled + %li + 1. + = link_to 'Add a slash command', 'https://my.slack.com/services/new/slash-commands' + in your Slack team with these options: - %hr + %hr - .help-form - .form-group - = label_tag nil, 'Command', class: 'col-sm-2 col-xs-12 control-label' - .col-sm-10.col-xs-12.text-block - %p Fill in the word that works best for your team. - %p - Suggestions: - %code= 'gitlab' - %code= @project.path # Path contains no spaces, but dashes - %code= @project.path_with_namespace + .help-form + .form-group + = label_tag nil, 'Command', class: 'col-sm-2 col-xs-12 control-label' + .col-sm-10.col-xs-12.text-block + %p Fill in the word that works best for your team. + %p + Suggestions: + %code= 'gitlab' + %code= @project.path # Path contains no spaces, but dashes + %code= @project.path_with_namespace - .form-group - = label_tag :url, 'URL', class: 'col-sm-2 col-xs-12 control-label' - .col-sm-10.col-xs-12.input-group - = text_field_tag :url, service_trigger_url(subject), class: 'form-control input-sm', readonly: 'readonly' - .input-group-btn - = clipboard_button(clipboard_target: '#url') + .form-group + = label_tag :url, 'URL', class: 'col-sm-2 col-xs-12 control-label' + .col-sm-10.col-xs-12.input-group + = text_field_tag :url, service_trigger_url(subject), class: 'form-control input-sm', readonly: 'readonly' + .input-group-btn + = clipboard_button(clipboard_target: '#url') - .form-group - = label_tag nil, 'Method', class: 'col-sm-2 col-xs-12 control-label' - .col-sm-10.col-xs-12.text-block POST + .form-group + = label_tag nil, 'Method', class: 'col-sm-2 col-xs-12 control-label' + .col-sm-10.col-xs-12.text-block POST - .form-group - = label_tag :customize_name, 'Customize name', class: 'col-sm-2 col-xs-12 control-label' - .col-sm-10.col-xs-12.input-group - = text_field_tag :customize_name, 'GitLab', class: 'form-control input-sm', readonly: 'readonly' - .input-group-btn - = clipboard_button(clipboard_target: '#customize_name') + .form-group + = label_tag :customize_name, 'Customize name', class: 'col-sm-2 col-xs-12 control-label' + .col-sm-10.col-xs-12.input-group + = text_field_tag :customize_name, 'GitLab', class: 'form-control input-sm', readonly: 'readonly' + .input-group-btn + = clipboard_button(clipboard_target: '#customize_name') - .form-group - = label_tag nil, 'Customize icon', class: 'col-sm-2 col-xs-12 control-label' - .col-sm-10.col-xs-12.text-block - = image_tag(asset_url('slash-command-logo.png'), width: 36, height: 36) - = link_to('Download image', asset_url('gitlab_logo.png'), class: 'btn btn-sm', target: '_blank') + .form-group + = label_tag nil, 'Customize icon', class: 'col-sm-2 col-xs-12 control-label' + .col-sm-10.col-xs-12.text-block + = image_tag(asset_url('slash-command-logo.png'), width: 36, height: 36) + = link_to('Download image', asset_url('gitlab_logo.png'), class: 'btn btn-sm', target: '_blank') - .form-group - = label_tag nil, 'Autocomplete', class: 'col-sm-2 col-xs-12 control-label' - .col-sm-10.col-xs-12.text-block Show this command in the autocomplete list + .form-group + = label_tag nil, 'Autocomplete', class: 'col-sm-2 col-xs-12 control-label' + .col-sm-10.col-xs-12.text-block Show this command in the autocomplete list - .form-group - = label_tag :autocomplete_description, 'Autocomplete description', class: 'col-sm-2 col-xs-12 control-label' - .col-sm-10.col-xs-12.input-group - = text_field_tag :autocomplete_description, run_actions_text, class: 'form-control input-sm', readonly: 'readonly' - .input-group-btn - = clipboard_button(clipboard_target: '#autocomplete_description') + .form-group + = label_tag :autocomplete_description, 'Autocomplete description', class: 'col-sm-2 col-xs-12 control-label' + .col-sm-10.col-xs-12.input-group + = text_field_tag :autocomplete_description, run_actions_text, class: 'form-control input-sm', readonly: 'readonly' + .input-group-btn + = clipboard_button(clipboard_target: '#autocomplete_description') - .form-group - = label_tag :autocomplete_usage_hint, 'Autocomplete usage hint', class: 'col-sm-2 col-xs-12 control-label' - .col-sm-10.col-xs-12.input-group - = text_field_tag :autocomplete_usage_hint, '[help]', class: 'form-control input-sm', readonly: 'readonly' - .input-group-btn - = clipboard_button(clipboard_target: '#autocomplete_usage_hint') + .form-group + = label_tag :autocomplete_usage_hint, 'Autocomplete usage hint', class: 'col-sm-2 col-xs-12 control-label' + .col-sm-10.col-xs-12.input-group + = text_field_tag :autocomplete_usage_hint, '[help]', class: 'form-control input-sm', readonly: 'readonly' + .input-group-btn + = clipboard_button(clipboard_target: '#autocomplete_usage_hint') - .form-group - = label_tag :descriptive_label, 'Descriptive label', class: 'col-sm-2 col-xs-12 control-label' - .col-sm-10.col-xs-12.input-group - = text_field_tag :descriptive_label, 'Perform common operations on GitLab project', class: 'form-control input-sm', readonly: 'readonly' - .input-group-btn - = clipboard_button(clipboard_target: '#descriptive_label') + .form-group + = label_tag :descriptive_label, 'Descriptive label', class: 'col-sm-2 col-xs-12 control-label' + .col-sm-10.col-xs-12.input-group + = text_field_tag :descriptive_label, 'Perform common operations on GitLab project', class: 'form-control input-sm', readonly: 'readonly' + .input-group-btn + = clipboard_button(clipboard_target: '#descriptive_label') - %hr + %hr - %ul.list-unstyled - %li - 2. Paste the - %strong Token - into the field below - %li - 3. Select the - %strong Active - checkbox, press - %strong Save changes - and start using GitLab inside Slack! + %ul.list-unstyled + %li + 2. Paste the + %strong Token + into the field below + %li + 3. Select the + %strong Active + checkbox, press + %strong Save changes + and start using GitLab inside Slack! -- cgit v1.2.1 From 932bcb4d32ee89922b6e7123a9c18489b36cc6e5 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Mon, 2 Jan 2017 10:25:28 +0000 Subject: Added isAuthenticate to differentiate between ineligible messages --- app/assets/javascripts/u2f/authenticate.js.es6 | 2 +- app/assets/javascripts/u2f/error.js | 16 ++++++++-------- app/assets/javascripts/u2f/register.js | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/u2f/authenticate.js.es6 b/app/assets/javascripts/u2f/authenticate.js.es6 index 2b992109a8c..f22d1f9b300 100644 --- a/app/assets/javascripts/u2f/authenticate.js.es6 +++ b/app/assets/javascripts/u2f/authenticate.js.es6 @@ -57,7 +57,7 @@ return function(response) { var error; if (response.errorCode) { - error = new U2FError(response.errorCode); + error = new U2FError(response.errorCode, 'authenticate'); return _this.renderError(error); } else { return _this.renderAuthenticated(JSON.stringify(response)); diff --git a/app/assets/javascripts/u2f/error.js b/app/assets/javascripts/u2f/error.js index bb9942a3aa0..50340d9174a 100644 --- a/app/assets/javascripts/u2f/error.js +++ b/app/assets/javascripts/u2f/error.js @@ -5,21 +5,21 @@ var bind = function(fn, me){ return function(){ return fn.apply(me, arguments); }; }; this.U2FError = (function() { - function U2FError(errorCode) { + function U2FError(errorCode, u2fFlowType) { this.errorCode = errorCode; this.message = bind(this.message, this); this.httpsDisabled = window.location.protocol !== 'https:'; + this.u2fFlowType = u2fFlowType; } U2FError.prototype.message = function() { - switch (false) { - case !(this.errorCode === u2f.ErrorCodes.BAD_REQUEST && this.httpsDisabled): - return "U2F only works with HTTPS-enabled websites. Contact your administrator for more details."; - case this.errorCode !== u2f.ErrorCodes.DEVICE_INELIGIBLE: - return "This device has already been registered with us."; - default: - return "There was a problem communicating with your device."; + if (this.errorCode === u2f.ErrorCodes.BAD_REQUEST && this.httpsDisabled) { + return 'U2F only works with HTTPS-enabled websites. Contact your administrator for more details.'; + } else if (this.errorCode === u2f.ErrorCodes.DEVICE_INELIGIBLE) { + if (this.u2fFlowType === 'authenticate') return 'This device has not been registered with us.'; + if (this.u2fFlowType === 'register') return 'This device has already been registered with us.'; } + return "There was a problem communicating with your device."; }; return U2FError; diff --git a/app/assets/javascripts/u2f/register.js b/app/assets/javascripts/u2f/register.js index 050c9bfc02e..92014afa593 100644 --- a/app/assets/javascripts/u2f/register.js +++ b/app/assets/javascripts/u2f/register.js @@ -39,7 +39,7 @@ return function(response) { var error; if (response.errorCode) { - error = new U2FError(response.errorCode); + error = new U2FError(response.errorCode, 'register'); return _this.renderError(error); } else { return _this.renderRegistered(JSON.stringify(response)); -- cgit v1.2.1 From 8c9a4ed373f4b517aeae669e64023dc52c8d704a Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 4 Jan 2017 17:46:56 +0800 Subject: WIP: Add tests and make sure that headers are set * We realized that headers were not set whenever we give 204 because `render_api_error!` doesn't preserve the headers. * We also realized that `update_runner_info` would be called in POST /builds/register every time therefore runner is updated every time, ticking the queue, making this last_update didn't work very well, and the test would be failing due to that. --- lib/api/helpers.rb | 2 +- lib/ci/api/builds.rb | 4 ++-- spec/models/build_spec.rb | 10 ++++++++++ spec/models/ci/runner_spec.rb | 33 +++++++++++++++++++++++++++++++++ spec/requests/ci/api/builds_spec.rb | 32 ++++++++++++++++++++++++++++++-- 5 files changed, 76 insertions(+), 5 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 746849ef4c0..3324001c468 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -229,7 +229,7 @@ module API end def render_api_error!(message, status) - error!({ 'message' => message }, status) + error!({ 'message' => message }, status, header) end def handle_api_exception(exception) diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index 8264210c460..de3e224bcee 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -17,7 +17,7 @@ module Ci update_runner_info if current_runner.is_runner_queue_value_latest?(params[:last_update]) - headers 'X-GitLab-Last-Update', params[:last_update] + header 'X-GitLab-Last-Update', params[:last_update] return build_not_found! end @@ -33,7 +33,7 @@ module Ci else Gitlab::Metrics.add_event(:build_not_found) - headers 'X-GitLab-Last-Update', new_update + header 'X-GitLab-Last-Update', new_update build_not_found! end diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb index d4970e38f7c..e902d9ef73d 100644 --- a/spec/models/build_spec.rb +++ b/spec/models/build_spec.rb @@ -1180,4 +1180,14 @@ describe Ci::Build, models: true do it { is_expected.to eq('review/master') } end end + + describe 'State transition: any => [:pending]' do + let(:build) { create(:ci_build, :created) } + + it 'queues BuildQueueWorker' do + expect(BuildQueueWorker).to receive(:perform_async).with(build.id) + + build.enqueue + end + end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index ef65eb99328..06eaa6d04d9 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -263,6 +263,39 @@ describe Ci::Runner, models: true do end end + describe '#tick_runner_queue' do + let(:runner) { create(:ci_runner) } + + it 'returns a new last_update value' do + expect(runner.tick_runner_queue).not_to be_empty + end + end + + describe '#ensure_runner_queue_value' do + let(:runner) { create(:ci_runner) } + + it 'sets a new last_update value when it is called the first time' do + last_update = runner.ensure_runner_queue_value + + expect_value_in_redis(last_update) + end + + it 'does not change if it is not expired and called again' do + last_update = runner.ensure_runner_queue_value + + expect(runner.ensure_runner_queue_value).to eq(last_update) + expect_value_in_redis(last_update) + end + + def expect_value_in_redis(last_update) + Gitlab::Redis.with do |redis| + runner_queue_key = runner.send(:runner_queue_key) + + expect(redis.get(runner_queue_key)).to eq(last_update) + end + end + end + describe '.assignable_for' do let(:runner) { create(:ci_runner) } let(:project) { create(:project) } diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index 80652129928..8ccae0d5bff 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -5,6 +5,7 @@ describe Ci::API::Builds do let(:runner) { FactoryGirl.create(:ci_runner, tag_list: ["mysql", "ruby"]) } let(:project) { FactoryGirl.create(:empty_project) } + let(:last_update) { nil } describe "Builds API for runners" do let(:pipeline) { create(:ci_pipeline_without_jobs, project: project, ref: 'master') } @@ -24,7 +25,31 @@ describe Ci::API::Builds do shared_examples 'no builds available' do context 'when runner sends version in User-Agent' do context 'for stable version' do - it { expect(response).to have_http_status(204) } + it 'gives 204 and set X-GitLab-Last-Update' do + expect(response).to have_http_status(204) + expect(response.header).to have_key('X-GitLab-Last-Update') + end + end + + context 'when last_update is up-to-date' do + let(:last_update) { runner.ensure_runner_queue_value } + + it 'gives 204 and set the same X-GitLab-Last-Update' do + expect(response).to have_http_status(204) + expect(response.header['X-GitLab-Last-Update']) + .to eq(last_update) + end + end + + context 'when last_update is outdated' do + let(:last_update) { runner.ensure_runner_queue_value } + let!(:new_update) { runner.tick_runner_queue } + + it 'gives 204 and set a new X-GitLab-Last-Update' do + expect(response).to have_http_status(204) + expect(response.header['X-GitLab-Last-Update']) + .to eq(new_update) + end end context 'for beta version' do @@ -44,6 +69,7 @@ describe Ci::API::Builds do register_builds info: { platform: :darwin } expect(response).to have_http_status(201) + expect(response.headers).not_to have_key('X-GitLab-Last-Update') expect(json_response['sha']).to eq(build.sha) expect(runner.reload.platform).to eq("darwin") expect(json_response["options"]).to eq({ "image" => "ruby:2.1", "services" => ["postgres"] }) @@ -219,7 +245,9 @@ describe Ci::API::Builds do end def register_builds(token = runner.token, **params) - post ci_api("/builds/register"), params.merge(token: token), { 'User-Agent' => user_agent } + new_params = params.merge(token: token, last_update: last_update) + + post ci_api("/builds/register"), new_params, { 'User-Agent' => user_agent } end end -- cgit v1.2.1 From df99883d4487e4a511a2b930c53c9d993bde8704 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Fri, 6 Jan 2017 13:21:49 +0100 Subject: Add tests for admin service templates --- .../services/slack_slash_commands/_help.html.haml | 2 +- spec/controllers/admin/services_controller_spec.rb | 26 ++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 spec/controllers/admin/services_controller_spec.rb diff --git a/app/views/projects/services/slack_slash_commands/_help.html.haml b/app/views/projects/services/slack_slash_commands/_help.html.haml index 8aac1b0c7da..04b9100acc6 100644 --- a/app/views/projects/services/slack_slash_commands/_help.html.haml +++ b/app/views/projects/services/slack_slash_commands/_help.html.haml @@ -1,4 +1,4 @@ -- pretty_name = defined?(@project) ? @project.path_with_namespace : "namespace / path" +- pretty_name = defined?(@project) ? @project.name_with_namespace : "namespace / path" - run_actions_text = "Perform common operations on this project: #{pretty_name}" .well diff --git a/spec/controllers/admin/services_controller_spec.rb b/spec/controllers/admin/services_controller_spec.rb new file mode 100644 index 00000000000..e5cdd52307e --- /dev/null +++ b/spec/controllers/admin/services_controller_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe Admin::ServicesController do + let(:admin) { create(:admin) } + + before { sign_in(admin) } + + describe 'GET #edit' do + let!(:project) { create(:empty_project) } + + Service.available_services_names.each do |service_name| + context "#{service_name}" do + let!(:service) do + service_template = service_name.concat("_service").camelize.constantize + service_template.where(template: true).first_or_create + end + + it 'successfully displays the template' do + get :edit, id: service.id + + expect(response).to have_http_status(200) + end + end + end + end +end -- cgit v1.2.1 From 43f9fc647de9c750bb93dc968e4ba3820d32b3f0 Mon Sep 17 00:00:00 2001 From: Kenneth Toley Date: Fri, 6 Jan 2017 23:06:16 +0000 Subject: Update enviroments.md the example for deleting an environment is missing the "s" in environments. curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environments/1" wil 404 --- doc/api/enviroments.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/enviroments.md b/doc/api/enviroments.md index 1299aca8c45..b0d2bc21814 100644 --- a/doc/api/enviroments.md +++ b/doc/api/enviroments.md @@ -106,7 +106,7 @@ DELETE /projects/:id/environments/:environment_id | `environment_id` | integer | yes | The ID of the environment | ```bash -curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environment/1" +curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environments/1" ``` Example response: -- cgit v1.2.1 From 8b30dd9834fd4026b846b016868701d8e95ec048 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 11:46:15 +0100 Subject: Extract abstract success with warnings CI/CD status --- lib/gitlab/ci/status/pipeline/factory.rb | 2 +- lib/gitlab/ci/status/pipeline/success_warning.rb | 13 ++++ .../ci/status/pipeline/success_with_warnings.rb | 31 ---------- lib/gitlab/ci/status/success_warning.rb | 35 +++++++++++ spec/lib/gitlab/ci/status/pipeline/factory_spec.rb | 2 +- .../ci/status/pipeline/success_warning_spec.rb | 69 ++++++++++++++++++++++ .../status/pipeline/success_with_warnings_spec.rb | 69 ---------------------- 7 files changed, 119 insertions(+), 102 deletions(-) create mode 100644 lib/gitlab/ci/status/pipeline/success_warning.rb delete mode 100644 lib/gitlab/ci/status/pipeline/success_with_warnings.rb create mode 100644 lib/gitlab/ci/status/success_warning.rb create mode 100644 spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb delete mode 100644 spec/lib/gitlab/ci/status/pipeline/success_with_warnings_spec.rb diff --git a/lib/gitlab/ci/status/pipeline/factory.rb b/lib/gitlab/ci/status/pipeline/factory.rb index 16dcb326be9..31e56a485b7 100644 --- a/lib/gitlab/ci/status/pipeline/factory.rb +++ b/lib/gitlab/ci/status/pipeline/factory.rb @@ -4,7 +4,7 @@ module Gitlab module Pipeline class Factory < Status::Factory def self.extended_statuses - [Pipeline::SuccessWithWarnings] + [Pipeline::SuccessWarning] end def self.common_helpers diff --git a/lib/gitlab/ci/status/pipeline/success_warning.rb b/lib/gitlab/ci/status/pipeline/success_warning.rb new file mode 100644 index 00000000000..8387682e744 --- /dev/null +++ b/lib/gitlab/ci/status/pipeline/success_warning.rb @@ -0,0 +1,13 @@ +module Gitlab + module Ci + module Status + module Pipeline + class SuccessWarning < Status::SuccessWarning + def self.matches?(pipeline, user) + pipeline.success? && pipeline.has_warnings? + end + end + end + end + end +end diff --git a/lib/gitlab/ci/status/pipeline/success_with_warnings.rb b/lib/gitlab/ci/status/pipeline/success_with_warnings.rb deleted file mode 100644 index 24bf8b869e0..00000000000 --- a/lib/gitlab/ci/status/pipeline/success_with_warnings.rb +++ /dev/null @@ -1,31 +0,0 @@ -module Gitlab - module Ci - module Status - module Pipeline - class SuccessWithWarnings < SimpleDelegator - include Status::Extended - - def text - 'passed' - end - - def label - 'passed with warnings' - end - - def icon - 'icon_status_warning' - end - - def group - 'success_with_warnings' - end - - def self.matches?(pipeline, user) - pipeline.success? && pipeline.has_warnings? - end - end - end - end - end -end diff --git a/lib/gitlab/ci/status/success_warning.rb b/lib/gitlab/ci/status/success_warning.rb new file mode 100644 index 00000000000..2affcc08f50 --- /dev/null +++ b/lib/gitlab/ci/status/success_warning.rb @@ -0,0 +1,35 @@ +module Gitlab + module Ci + module Status + ## + # Abstract extended status used when pipeline/stage/build passed + # conditionally. + # + # This means that failed jobs that are allowed to fail were present. + # + class SuccessWarning < SimpleDelegator + include Status::Extended + + def text + 'passed' + end + + def label + 'passed with warnings' + end + + def icon + 'icon_status_warning' + end + + def group + 'success_with_warnings' + end + + def self.matches?(subject, user) + raise NotImplementedError + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb index d4a2dc7fcc1..672f5733e98 100644 --- a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb @@ -49,7 +49,7 @@ describe Gitlab::Ci::Status::Pipeline::Factory do it 'fabricates extended "success with warnings" status' do expect(status) - .to be_a Gitlab::Ci::Status::Pipeline::SuccessWithWarnings + .to be_a Gitlab::Ci::Status::Pipeline::SuccessWarning end it 'extends core status with common pipeline methods' do diff --git a/spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb b/spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb new file mode 100644 index 00000000000..ee3f6174b73 --- /dev/null +++ b/spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb @@ -0,0 +1,69 @@ +require 'spec_helper' + +describe Gitlab::Ci::Status::Pipeline::SuccessWarning do + subject do + described_class.new(double('status')) + end + + describe '#test' do + it { expect(subject.text).to eq 'passed' } + end + + describe '#label' do + it { expect(subject.label).to eq 'passed with warnings' } + end + + describe '#icon' do + it { expect(subject.icon).to eq 'icon_status_warning' } + end + + describe '#group' do + it { expect(subject.group).to eq 'success_with_warnings' } + end + + describe '.matches?' do + context 'when pipeline is successful' do + let(:pipeline) do + create(:ci_pipeline, status: :success) + end + + context 'when pipeline has warnings' do + before do + allow(pipeline).to receive(:has_warnings?).and_return(true) + end + + it 'is a correct match' do + expect(described_class.matches?(pipeline, double)).to eq true + end + end + + context 'when pipeline does not have warnings' do + it 'does not match' do + expect(described_class.matches?(pipeline, double)).to eq false + end + end + end + + context 'when pipeline is not successful' do + let(:pipeline) do + create(:ci_pipeline, status: :skipped) + end + + context 'when pipeline has warnings' do + before do + allow(pipeline).to receive(:has_warnings?).and_return(true) + end + + it 'does not match' do + expect(described_class.matches?(pipeline, double)).to eq false + end + end + + context 'when pipeline does not have warnings' do + it 'does not match' do + expect(described_class.matches?(pipeline, double)).to eq false + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/status/pipeline/success_with_warnings_spec.rb b/spec/lib/gitlab/ci/status/pipeline/success_with_warnings_spec.rb deleted file mode 100644 index 979160eb9c4..00000000000 --- a/spec/lib/gitlab/ci/status/pipeline/success_with_warnings_spec.rb +++ /dev/null @@ -1,69 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Ci::Status::Pipeline::SuccessWithWarnings do - subject do - described_class.new(double('status')) - end - - describe '#test' do - it { expect(subject.text).to eq 'passed' } - end - - describe '#label' do - it { expect(subject.label).to eq 'passed with warnings' } - end - - describe '#icon' do - it { expect(subject.icon).to eq 'icon_status_warning' } - end - - describe '#group' do - it { expect(subject.group).to eq 'success_with_warnings' } - end - - describe '.matches?' do - context 'when pipeline is successful' do - let(:pipeline) do - create(:ci_pipeline, status: :success) - end - - context 'when pipeline has warnings' do - before do - allow(pipeline).to receive(:has_warnings?).and_return(true) - end - - it 'is a correct match' do - expect(described_class.matches?(pipeline, double)).to eq true - end - end - - context 'when pipeline does not have warnings' do - it 'does not match' do - expect(described_class.matches?(pipeline, double)).to eq false - end - end - end - - context 'when pipeline is not successful' do - let(:pipeline) do - create(:ci_pipeline, status: :skipped) - end - - context 'when pipeline has warnings' do - before do - allow(pipeline).to receive(:has_warnings?).and_return(true) - end - - it 'does not match' do - expect(described_class.matches?(pipeline, double)).to eq false - end - end - - context 'when pipeline does not have warnings' do - it 'does not match' do - expect(described_class.matches?(pipeline, double)).to eq false - end - end - end - end -end -- cgit v1.2.1 From 8dbd1e7d0000bb08b6ac6867530bb501eadc85a4 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 12:22:19 +0100 Subject: Add concrete success warning status to stage factory --- app/models/ci/stage.rb | 8 +++ lib/gitlab/ci/status/pipeline/factory.rb | 2 +- lib/gitlab/ci/status/pipeline/success_warning.rb | 13 ---- lib/gitlab/ci/status/stage/factory.rb | 4 ++ lib/gitlab/ci/status/success_warning.rb | 6 +- spec/lib/gitlab/ci/status/pipeline/factory_spec.rb | 5 +- .../ci/status/pipeline/success_warning_spec.rb | 69 -------------------- spec/lib/gitlab/ci/status/stage/factory_spec.rb | 22 +++++++ spec/lib/gitlab/ci/status/success_warning_spec.rb | 75 ++++++++++++++++++++++ 9 files changed, 115 insertions(+), 89 deletions(-) delete mode 100644 lib/gitlab/ci/status/pipeline/success_warning.rb delete mode 100644 spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb create mode 100644 spec/lib/gitlab/ci/status/success_warning_spec.rb diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index d035eda6df5..d4b6ff910aa 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -39,5 +39,13 @@ module Ci def builds @builds ||= pipeline.builds.where(stage: name) end + + def success? + status.to_s == 'success' + end + + def has_warnings? + statuses.latest.failed_but_allowed.any? + end end end diff --git a/lib/gitlab/ci/status/pipeline/factory.rb b/lib/gitlab/ci/status/pipeline/factory.rb index 31e56a485b7..13c8343b12a 100644 --- a/lib/gitlab/ci/status/pipeline/factory.rb +++ b/lib/gitlab/ci/status/pipeline/factory.rb @@ -4,7 +4,7 @@ module Gitlab module Pipeline class Factory < Status::Factory def self.extended_statuses - [Pipeline::SuccessWarning] + [Status::SuccessWarning] end def self.common_helpers diff --git a/lib/gitlab/ci/status/pipeline/success_warning.rb b/lib/gitlab/ci/status/pipeline/success_warning.rb deleted file mode 100644 index 8387682e744..00000000000 --- a/lib/gitlab/ci/status/pipeline/success_warning.rb +++ /dev/null @@ -1,13 +0,0 @@ -module Gitlab - module Ci - module Status - module Pipeline - class SuccessWarning < Status::SuccessWarning - def self.matches?(pipeline, user) - pipeline.success? && pipeline.has_warnings? - end - end - end - end - end -end diff --git a/lib/gitlab/ci/status/stage/factory.rb b/lib/gitlab/ci/status/stage/factory.rb index 689a5dd45bc..4c37f084d07 100644 --- a/lib/gitlab/ci/status/stage/factory.rb +++ b/lib/gitlab/ci/status/stage/factory.rb @@ -3,6 +3,10 @@ module Gitlab module Status module Stage class Factory < Status::Factory + def self.extended_statuses + [Status::SuccessWarning] + end + def self.common_helpers Status::Stage::Common end diff --git a/lib/gitlab/ci/status/success_warning.rb b/lib/gitlab/ci/status/success_warning.rb index 2affcc08f50..d4cdab6957a 100644 --- a/lib/gitlab/ci/status/success_warning.rb +++ b/lib/gitlab/ci/status/success_warning.rb @@ -2,9 +2,7 @@ module Gitlab module Ci module Status ## - # Abstract extended status used when pipeline/stage/build passed - # conditionally. - # + # Extended status used when pipeline or stage passed conditionally. # This means that failed jobs that are allowed to fail were present. # class SuccessWarning < SimpleDelegator @@ -27,7 +25,7 @@ module Gitlab end def self.matches?(subject, user) - raise NotImplementedError + subject.success? && subject.has_warnings? end end end diff --git a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb index 672f5733e98..0df6e881877 100644 --- a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb @@ -49,11 +49,12 @@ describe Gitlab::Ci::Status::Pipeline::Factory do it 'fabricates extended "success with warnings" status' do expect(status) - .to be_a Gitlab::Ci::Status::Pipeline::SuccessWarning + .to be_a Gitlab::Ci::Status::SuccessWarning end - it 'extends core status with common pipeline methods' do + it 'extends core status with common pipeline method' do expect(status).to have_details + expect(status.details_path).to include "pipelines/#{pipeline.id}" end end end diff --git a/spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb b/spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb deleted file mode 100644 index ee3f6174b73..00000000000 --- a/spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb +++ /dev/null @@ -1,69 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Ci::Status::Pipeline::SuccessWarning do - subject do - described_class.new(double('status')) - end - - describe '#test' do - it { expect(subject.text).to eq 'passed' } - end - - describe '#label' do - it { expect(subject.label).to eq 'passed with warnings' } - end - - describe '#icon' do - it { expect(subject.icon).to eq 'icon_status_warning' } - end - - describe '#group' do - it { expect(subject.group).to eq 'success_with_warnings' } - end - - describe '.matches?' do - context 'when pipeline is successful' do - let(:pipeline) do - create(:ci_pipeline, status: :success) - end - - context 'when pipeline has warnings' do - before do - allow(pipeline).to receive(:has_warnings?).and_return(true) - end - - it 'is a correct match' do - expect(described_class.matches?(pipeline, double)).to eq true - end - end - - context 'when pipeline does not have warnings' do - it 'does not match' do - expect(described_class.matches?(pipeline, double)).to eq false - end - end - end - - context 'when pipeline is not successful' do - let(:pipeline) do - create(:ci_pipeline, status: :skipped) - end - - context 'when pipeline has warnings' do - before do - allow(pipeline).to receive(:has_warnings?).and_return(true) - end - - it 'does not match' do - expect(described_class.matches?(pipeline, double)).to eq false - end - end - - context 'when pipeline does not have warnings' do - it 'does not match' do - expect(described_class.matches?(pipeline, double)).to eq false - end - end - end - end -end diff --git a/spec/lib/gitlab/ci/status/stage/factory_spec.rb b/spec/lib/gitlab/ci/status/stage/factory_spec.rb index 6f8721d30c2..a60f84be9e9 100644 --- a/spec/lib/gitlab/ci/status/stage/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/stage/factory_spec.rb @@ -42,5 +42,27 @@ describe Gitlab::Ci::Status::Stage::Factory do end end end + + end + + context 'when stage has warnings' do + let(:stage) do + build(:ci_stage, name: 'test', status: :success, pipeline: pipeline) + end + + before do + create(:ci_build, :allowed_to_fail, :failed, + stage: 'test', pipeline: stage.pipeline) + end + + it 'fabricates extended "success with warnings" status' do + expect(status) + .to be_a Gitlab::Ci::Status::SuccessWarning + end + + it 'extends core status with common stage method' do + expect(status).to have_details + expect(status.details_path).to include "pipelines/#{pipeline.id}##{stage.name}" + end end end diff --git a/spec/lib/gitlab/ci/status/success_warning_spec.rb b/spec/lib/gitlab/ci/status/success_warning_spec.rb new file mode 100644 index 00000000000..7e2269397c6 --- /dev/null +++ b/spec/lib/gitlab/ci/status/success_warning_spec.rb @@ -0,0 +1,75 @@ +require 'spec_helper' + +describe Gitlab::Ci::Status::SuccessWarning do + subject do + described_class.new(double('status')) + end + + describe '#test' do + it { expect(subject.text).to eq 'passed' } + end + + describe '#label' do + it { expect(subject.label).to eq 'passed with warnings' } + end + + describe '#icon' do + it { expect(subject.icon).to eq 'icon_status_warning' } + end + + describe '#group' do + it { expect(subject.group).to eq 'success_with_warnings' } + end + + describe '.matches?' do + let(:matchable) { double('matchable') } + + context 'when matchable subject is successful' do + before do + allow(matchable).to receive(:success?).and_return(true) + end + + context 'when matchable subject has warnings' do + before do + allow(matchable).to receive(:has_warnings?).and_return(true) + end + + it 'is a correct match' do + expect(described_class.matches?(matchable, double)).to eq true + end + end + + context 'when matchable subject does not have warnings' do + before do + allow(matchable).to receive(:has_warnings?).and_return(false) + end + + it 'does not match' do + expect(described_class.matches?(matchable, double)).to eq false + end + end + end + + context 'when matchable subject is not successful' do + before do + allow(matchable).to receive(:success?).and_return(false) + end + + context 'when matchable subject has warnings' do + before do + allow(matchable).to receive(:has_warnings?).and_return(true) + end + + it 'does not match' do + expect(described_class.matches?(matchable, double)).to eq false + end + end + + context 'when matchable subject does not have warnings' do + it 'does not match' do + expect(described_class.matches?(matchable, double)).to eq false + end + end + end + end +end -- cgit v1.2.1 From fd115bc130a8bf77814262b67eb0f20f1f19cb85 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 13:07:52 +0100 Subject: Add specs for two new methods defined in stage class --- spec/lib/gitlab/ci/status/stage/factory_spec.rb | 1 - spec/models/ci/stage_spec.rb | 48 +++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/spec/lib/gitlab/ci/status/stage/factory_spec.rb b/spec/lib/gitlab/ci/status/stage/factory_spec.rb index a60f84be9e9..bbb40e2c1ab 100644 --- a/spec/lib/gitlab/ci/status/stage/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/stage/factory_spec.rb @@ -42,7 +42,6 @@ describe Gitlab::Ci::Status::Stage::Factory do end end end - end context 'when stage has warnings' do diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index 742bedb37e4..3d387d52c8e 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -142,6 +142,54 @@ describe Ci::Stage, models: true do end end + describe '#success?' do + context 'when stage is successful' do + before do + create_job(:ci_build, status: :success) + create_job(:generic_commit_status, status: :success) + end + + it 'is successful' do + expect(stage).to be_success + end + end + + context 'when stage is not successful' do + before do + create_job(:ci_build, status: :failed) + create_job(:generic_commit_status, status: :success) + end + + it 'is not successful' do + expect(stage).not_to be_success + end + end + end + + describe '#has_warnings?' do + context 'when stage has warnings' do + before do + create(:ci_build, :failed, :allowed_to_fail, + stage: stage_name, pipeline: pipeline) + end + + it 'has warnings' do + expect(stage).to have_warnings + end + end + + context 'when stage does not have warnings' do + before do + create(:ci_build, :success, stage: stage_name, + pipeline: pipeline) + end + + it 'does not have warnings' do + expect(stage).not_to have_warnings + end + end + end + def create_job(type, status: 'success', stage: stage_name) create(type, pipeline: pipeline, stage: stage, status: status) end -- cgit v1.2.1 From e5d53df70e6bebf267e5709790842cb674ee96b0 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 14:13:16 +0100 Subject: Add changelog for warning icon for stage in graph --- .../unreleased/feature-success-warning-icons-in-stages-builds.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/feature-success-warning-icons-in-stages-builds.yml diff --git a/changelogs/unreleased/feature-success-warning-icons-in-stages-builds.yml b/changelogs/unreleased/feature-success-warning-icons-in-stages-builds.yml new file mode 100644 index 00000000000..5fba0332881 --- /dev/null +++ b/changelogs/unreleased/feature-success-warning-icons-in-stages-builds.yml @@ -0,0 +1,4 @@ +--- +title: Use warning icon in mini-graph if stage passed conditionally +merge_request: 8503 +author: -- cgit v1.2.1 From 9f1279184b85927a12b93df00896c57b12373a9a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 11 Jan 2017 13:26:56 +0100 Subject: Add extended status for build failed but allowed to --- lib/gitlab/ci/status/build/failed_allowed.rb | 27 +++++ .../gitlab/ci/status/build/failed_allowed_spec.rb | 110 +++++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 lib/gitlab/ci/status/build/failed_allowed.rb create mode 100644 spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb diff --git a/lib/gitlab/ci/status/build/failed_allowed.rb b/lib/gitlab/ci/status/build/failed_allowed.rb new file mode 100644 index 00000000000..807afe24bd5 --- /dev/null +++ b/lib/gitlab/ci/status/build/failed_allowed.rb @@ -0,0 +1,27 @@ +module Gitlab + module Ci + module Status + module Build + class FailedAllowed < SimpleDelegator + include Status::Extended + + def label + 'failed (allowed to fail)' + end + + def icon + 'icon_status_warning' + end + + def group + 'failed_with_warnings' + end + + def self.matches?(build, user) + build.failed? && build.allow_failure? + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb b/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb new file mode 100644 index 00000000000..20f71459738 --- /dev/null +++ b/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb @@ -0,0 +1,110 @@ +require 'spec_helper' + +describe Gitlab::Ci::Status::Build::FailedAllowed do + let(:status) { double('core status') } + let(:user) { double('user') } + + subject do + described_class.new(status) + end + + describe '#text' do + it 'does not override status text' do + expect(status).to receive(:text) + + subject.text + end + end + + describe '#icon' do + it 'returns a warning icon' do + expect(subject.icon).to eq 'icon_status_warning' + end + end + + describe '#label' do + it 'returns information about failed but allowed to fail status' do + expect(subject.label).to eq 'failed (allowed to fail)' + end + end + + describe '#group' do + it 'returns status failed with warnings status group' do + expect(subject.group).to eq 'failed_with_warnings' + end + end + + describe 'action details' do + describe '#has_action?' do + it 'does not decorate action details' do + expect(status).to receive(:has_action?) + + subject.has_action? + end + end + + describe '#action_path' do + it 'does not decorate action path' do + expect(status).to receive(:action_path) + + subject.action_path + end + end + + describe '#action_icon' do + it 'does not decorate action icon' do + expect(status).to receive(:action_icon) + + subject.action_icon + end + end + + describe '#action_title' do + it 'does not decorate action title' do + expect(status).to receive(:action_title) + + subject.action_title + end + end + end + + describe '.matches?' do + subject { described_class.matches?(build, user) } + + context 'when build is failed' do + context 'when build is allowed to fail' do + let(:build) { create(:ci_build, :failed, :allowed_to_fail) } + + it 'is a correct match' do + expect(subject).to be true + end + end + + context 'when build is not allowed to fail' do + let(:build) { create(:ci_build, :failed) } + + it 'is not a correct match' do + expect(subject).not_to be true + end + end + end + + context 'when build did not fail' do + context 'when build is allowed to fail' do + let(:build) { create(:ci_build, :success, :allowed_to_fail) } + + it 'is not a correct match' do + expect(subject).not_to be true + end + end + + context 'when build is not allowed to fail' do + let(:build) { create(:ci_build, :success) } + + it 'is not a correct match' do + expect(subject).not_to be true + end + end + end + end +end -- cgit v1.2.1 From 6b72688b88cc304482f465ff4e6066a260b20b4e Mon Sep 17 00:00:00 2001 From: Koen Punt Date: Wed, 11 Jan 2017 15:32:51 +0000 Subject: Fix typo in examples --- doc/api/enviroments.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/enviroments.md b/doc/api/enviroments.md index 1299aca8c45..e0ee20d9610 100644 --- a/doc/api/enviroments.md +++ b/doc/api/enviroments.md @@ -78,7 +78,7 @@ PUT /projects/:id/environments/:environments_id | `external_url` | string | no | The new external_url | ```bash -curl --request PUT --data "name=staging&external_url=https://staging.example.gitlab.com" --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environment/1" +curl --request PUT --data "name=staging&external_url=https://staging.example.gitlab.com" --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environments/1" ``` Example response: @@ -106,7 +106,7 @@ DELETE /projects/:id/environments/:environment_id | `environment_id` | integer | yes | The ID of the environment | ```bash -curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environment/1" +curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environments/1" ``` Example response: -- cgit v1.2.1 From 1d01ffb782a1bf44d5826666590408b24fba2332 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 12:45:00 +0100 Subject: Make it possible to combine extended CI/CD statuses This commit also makes it possible to configure exclusive groups. There can be only one detailed status matched within an exclusive group, which is important from the performance perspective. --- lib/gitlab/ci/status/factory.rb | 18 ++++-- spec/lib/gitlab/ci/status/factory_spec.rb | 97 ++++++++++++++++++++++++++++--- 2 files changed, 102 insertions(+), 13 deletions(-) diff --git a/lib/gitlab/ci/status/factory.rb b/lib/gitlab/ci/status/factory.rb index ae9ef895df4..71c54aebcc3 100644 --- a/lib/gitlab/ci/status/factory.rb +++ b/lib/gitlab/ci/status/factory.rb @@ -8,10 +8,12 @@ module Gitlab end def fabricate! - if extended_status - extended_status.new(core_status) - else + if extended_statuses.none? core_status + else + extended_statuses.inject(core_status) do |status, extended| + extended.new(status) + end end end @@ -36,10 +38,14 @@ module Gitlab .extend(self.class.common_helpers) end - def extended_status - @extended ||= self.class.extended_statuses.find do |status| - status.matches?(@subject, @user) + def extended_statuses + return @extended_statuses if defined?(@extended_statuses) + + groups = self.class.extended_statuses.map do |group| + Array(group).find { |status| status.matches?(@subject, @user) } end + + @extended_statuses = groups.flatten.compact end end end diff --git a/spec/lib/gitlab/ci/status/factory_spec.rb b/spec/lib/gitlab/ci/status/factory_spec.rb index f92a1c149bf..d78d563a9b9 100644 --- a/spec/lib/gitlab/ci/status/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/factory_spec.rb @@ -1,18 +1,14 @@ require 'spec_helper' describe Gitlab::Ci::Status::Factory do - subject do - described_class.new(resource, user) - end - let(:user) { create(:user) } - - let(:status) { subject.fabricate! } + let(:status) { factory.fabricate! } + let(:factory) { described_class.new(resource, user) } context 'when object has a core status' do HasStatus::AVAILABLE_STATUSES.each do |core_status| context "when core status is #{core_status}" do - let(:resource) { double(status: core_status) } + let(:resource) { double('resource', status: core_status) } it "fabricates a core status #{core_status}" do expect(status).to be_a( @@ -21,4 +17,91 @@ describe Gitlab::Ci::Status::Factory do end end end + + context 'when resource supports multiple extended statuses' do + let(:resource) { double('resource', status: :success) } + + let(:first_extended_status) do + Class.new(SimpleDelegator) do + def first_method + 'first return value' + end + + def second_method + 'second return value' + end + + def self.matches?(*) + true + end + end + end + + let(:second_extended_status) do + Class.new(SimpleDelegator) do + def first_method + 'decorated return value' + end + + def third_method + 'third return value' + end + + def self.matches?(*) + true + end + end + end + + shared_examples 'compound decorator factory' do + it 'fabricates compound decorator' do + expect(status.first_method).to eq 'decorated return value' + expect(status.second_method).to eq 'second return value' + expect(status.third_method).to eq 'third return value' + end + + it 'delegates to core status' do + expect(status.text).to eq 'passed' + end + + it 'latest matches status becomes a status name' do + expect(status.class).to eq second_extended_status + end + end + + context 'when exclusive statuses are matches' do + before do + allow(described_class).to receive(:extended_statuses) + .and_return([[first_extended_status, second_extended_status]]) + end + + it 'fabricates compound decorator' do + expect(status.first_method).to eq 'first return value' + expect(status.second_method).to eq 'second return value' + expect(status).not_to respond_to(:third_method) + end + + it 'delegates to core status' do + expect(status.text).to eq 'passed' + end + end + + context 'when exclusive statuses are not matched' do + before do + allow(described_class).to receive(:extended_statuses) + .and_return([[first_extended_status], [second_extended_status]]) + end + + it_behaves_like 'compound decorator factory' + end + + context 'when using simplified status grouping' do + before do + allow(described_class).to receive(:extended_statuses) + .and_return([first_extended_status, second_extended_status]) + end + + it_behaves_like 'compound decorator factory' + end + end end -- cgit v1.2.1 From 48c19e7395e501c8e9eaa3975b2510b37f56d46c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:04:51 +0100 Subject: Expose methods that match statuses in status factories --- lib/gitlab/ci/status/factory.rb | 25 +++++-------- spec/lib/gitlab/ci/status/factory_spec.rb | 62 ++++++++++++++++++++++--------- 2 files changed, 55 insertions(+), 32 deletions(-) diff --git a/lib/gitlab/ci/status/factory.rb b/lib/gitlab/ci/status/factory.rb index 71c54aebcc3..3da5ae4fc01 100644 --- a/lib/gitlab/ci/status/factory.rb +++ b/lib/gitlab/ci/status/factory.rb @@ -5,6 +5,7 @@ module Gitlab def initialize(subject, user) @subject = subject @user = user + @status = subject.status || :created end def fabricate! @@ -17,23 +18,9 @@ module Gitlab end end - def self.extended_statuses - [] - end - - def self.common_helpers - Module.new - end - - private - - def simple_status - @simple_status ||= @subject.status || :created - end - def core_status Gitlab::Ci::Status - .const_get(simple_status.capitalize) + .const_get(@status.capitalize) .new(@subject, @user) .extend(self.class.common_helpers) end @@ -47,6 +34,14 @@ module Gitlab @extended_statuses = groups.flatten.compact end + + def self.extended_statuses + [] + end + + def self.common_helpers + Module.new + end end end end diff --git a/spec/lib/gitlab/ci/status/factory_spec.rb b/spec/lib/gitlab/ci/status/factory_spec.rb index d78d563a9b9..f1b758640a7 100644 --- a/spec/lib/gitlab/ci/status/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/factory_spec.rb @@ -2,17 +2,28 @@ require 'spec_helper' describe Gitlab::Ci::Status::Factory do let(:user) { create(:user) } - let(:status) { factory.fabricate! } + let(:fabricated_status) { factory.fabricate! } let(:factory) { described_class.new(resource, user) } context 'when object has a core status' do - HasStatus::AVAILABLE_STATUSES.each do |core_status| - context "when core status is #{core_status}" do - let(:resource) { double('resource', status: core_status) } + HasStatus::AVAILABLE_STATUSES.each do |simple_status| + context "when simple core status is #{simple_status}" do + let(:resource) { double('resource', status: simple_status) } - it "fabricates a core status #{core_status}" do - expect(status).to be_a( - Gitlab::Ci::Status.const_get(core_status.capitalize)) + let(:expected_status) do + Gitlab::Ci::Status.const_get(simple_status.capitalize) + end + + it "fabricates a core status #{simple_status}" do + expect(fabricated_status).to be_a expected_status + end + + it "matches a valid core status for #{simple_status}" do + expect(factory.core_status).to be_a expected_status + end + + it "does not match any extended statuses for #{simple_status}" do + expect(factory.extended_statuses).to be_empty end end end @@ -55,17 +66,26 @@ describe Gitlab::Ci::Status::Factory do shared_examples 'compound decorator factory' do it 'fabricates compound decorator' do - expect(status.first_method).to eq 'decorated return value' - expect(status.second_method).to eq 'second return value' - expect(status.third_method).to eq 'third return value' + expect(fabricated_status.first_method).to eq 'decorated return value' + expect(fabricated_status.second_method).to eq 'second return value' + expect(fabricated_status.third_method).to eq 'third return value' end it 'delegates to core status' do - expect(status.text).to eq 'passed' + expect(fabricated_status.text).to eq 'passed' end it 'latest matches status becomes a status name' do - expect(status.class).to eq second_extended_status + expect(fabricated_status.class).to eq second_extended_status + end + + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Success + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [first_extended_status, second_extended_status] end end @@ -75,14 +95,22 @@ describe Gitlab::Ci::Status::Factory do .and_return([[first_extended_status, second_extended_status]]) end - it 'fabricates compound decorator' do - expect(status.first_method).to eq 'first return value' - expect(status.second_method).to eq 'second return value' - expect(status).not_to respond_to(:third_method) + it 'does not fabricate compound decorator' do + expect(fabricated_status.first_method).to eq 'first return value' + expect(fabricated_status.second_method).to eq 'second return value' + expect(fabricated_status).not_to respond_to(:third_method) end it 'delegates to core status' do - expect(status.text).to eq 'passed' + expect(fabricated_status.text).to eq 'passed' + end + + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Success + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses).to eq [first_extended_status] end end -- cgit v1.2.1 From 6053049f4a8822bf39bbe8e1fa34c5e3522b63e9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:22:04 +0100 Subject: Add new CI/CD status failed with warnings icon group --- app/assets/stylesheets/framework/icons.scss | 1 + app/assets/stylesheets/pages/status.scss | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/framework/icons.scss b/app/assets/stylesheets/framework/icons.scss index dccf5177e35..868f28cd356 100644 --- a/app/assets/stylesheets/framework/icons.scss +++ b/app/assets/stylesheets/framework/icons.scss @@ -15,6 +15,7 @@ } .ci-status-icon-pending, +.ci-status-icon-failed_with_warnings, .ci-status-icon-success_with_warnings { color: $gl-warning; diff --git a/app/assets/stylesheets/pages/status.scss b/app/assets/stylesheets/pages/status.scss index f19275770be..6f31d4ed789 100644 --- a/app/assets/stylesheets/pages/status.scss +++ b/app/assets/stylesheets/pages/status.scss @@ -19,7 +19,8 @@ overflow: visible; } - &.ci-failed { + &.ci-failed, + &.ci-failed_with_warnings { color: $gl-danger; border-color: $gl-danger; -- cgit v1.2.1 From 227cbdd8ba969aecdbee68f1c892c85ed196d64e Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:22:46 +0100 Subject: Use detailed status for failed but allowed builds --- lib/gitlab/ci/status/build/factory.rb | 7 +- spec/lib/gitlab/ci/status/build/factory_spec.rb | 125 +++++++++++++++++++++--- 2 files changed, 118 insertions(+), 14 deletions(-) diff --git a/lib/gitlab/ci/status/build/factory.rb b/lib/gitlab/ci/status/build/factory.rb index eee9a64120b..38ac6edc9f1 100644 --- a/lib/gitlab/ci/status/build/factory.rb +++ b/lib/gitlab/ci/status/build/factory.rb @@ -4,8 +4,11 @@ module Gitlab module Build class Factory < Status::Factory def self.extended_statuses - [Status::Build::Stop, Status::Build::Play, - Status::Build::Cancelable, Status::Build::Retryable] + [[Status::Build::Cancelable, + Status::Build::Retryable], + [Status::Build::FailedAllowed, + Status::Build::Play, + Status::Build::Stop]] end def self.common_helpers diff --git a/spec/lib/gitlab/ci/status/build/factory_spec.rb b/spec/lib/gitlab/ci/status/build/factory_spec.rb index dccb29b5ef6..0c40fca0c1a 100644 --- a/spec/lib/gitlab/ci/status/build/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/build/factory_spec.rb @@ -3,15 +3,23 @@ require 'spec_helper' describe Gitlab::Ci::Status::Build::Factory do let(:user) { create(:user) } let(:project) { build.project } - - subject { described_class.new(build, user) } - let(:status) { subject.fabricate! } + let(:status) { factory.fabricate! } + let(:factory) { described_class.new(build, user) } before { project.team << [user, :developer] } context 'when build is successful' do let(:build) { create(:ci_build, :success) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Success + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Retryable] + end + it 'fabricates a retryable build status' do expect(status).to be_a Gitlab::Ci::Status::Build::Retryable end @@ -26,24 +34,72 @@ describe Gitlab::Ci::Status::Build::Factory do end context 'when build is failed' do - let(:build) { create(:ci_build, :failed) } + context 'when build is not allowed to fail' do + let(:build) { create(:ci_build, :failed) } - it 'fabricates a retryable build status' do - expect(status).to be_a Gitlab::Ci::Status::Build::Retryable + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Failed + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Retryable] + end + + it 'fabricates a retryable build status' do + expect(status).to be_a Gitlab::Ci::Status::Build::Retryable + end + + it 'fabricates status with correct details' do + expect(status.text).to eq 'failed' + expect(status.icon).to eq 'icon_status_failed' + expect(status.label).to eq 'failed' + expect(status).to have_details + expect(status).to have_action + end end - it 'fabricates status with correct details' do - expect(status.text).to eq 'failed' - expect(status.icon).to eq 'icon_status_failed' - expect(status.label).to eq 'failed' - expect(status).to have_details - expect(status).to have_action + context 'when build is allowed to fail' do + let(:build) { create(:ci_build, :failed, :allowed_to_fail) } + + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Failed + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Retryable, + Gitlab::Ci::Status::Build::FailedAllowed] + end + + it 'fabricates a failed but allowed build status' do + expect(status).to be_a Gitlab::Ci::Status::Build::FailedAllowed + end + + it 'fabricates status with correct details' do + expect(status.text).to eq 'failed' + expect(status.icon).to eq 'icon_status_warning' + expect(status.label).to eq 'failed (allowed to fail)' + expect(status).to have_details + expect(status).to have_action + expect(status.action_title).to include 'Retry' + expect(status.action_path).to include 'retry' + end end end context 'when build is a canceled' do let(:build) { create(:ci_build, :canceled) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Canceled + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Retryable] + end + it 'fabricates a retryable build status' do expect(status).to be_a Gitlab::Ci::Status::Build::Retryable end @@ -60,6 +116,15 @@ describe Gitlab::Ci::Status::Build::Factory do context 'when build is running' do let(:build) { create(:ci_build, :running) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Running + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Cancelable] + end + it 'fabricates a canceable build status' do expect(status).to be_a Gitlab::Ci::Status::Build::Cancelable end @@ -76,6 +141,15 @@ describe Gitlab::Ci::Status::Build::Factory do context 'when build is pending' do let(:build) { create(:ci_build, :pending) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Pending + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Cancelable] + end + it 'fabricates a cancelable build status' do expect(status).to be_a Gitlab::Ci::Status::Build::Cancelable end @@ -92,6 +166,14 @@ describe Gitlab::Ci::Status::Build::Factory do context 'when build is skipped' do let(:build) { create(:ci_build, :skipped) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Skipped + end + + it 'does not match extended statuses' do + expect(factory.extended_statuses).to be_empty + end + it 'fabricates a core skipped status' do expect(status).to be_a Gitlab::Ci::Status::Skipped end @@ -109,6 +191,15 @@ describe Gitlab::Ci::Status::Build::Factory do context 'when build is a play action' do let(:build) { create(:ci_build, :playable) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Skipped + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Play] + end + it 'fabricates a core skipped status' do expect(status).to be_a Gitlab::Ci::Status::Build::Play end @@ -119,12 +210,22 @@ describe Gitlab::Ci::Status::Build::Factory do expect(status.label).to eq 'manual play action' expect(status).to have_details expect(status).to have_action + expect(status.action_path).to include 'play' end end context 'when build is an environment stop action' do let(:build) { create(:ci_build, :playable, :teardown_environment) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Skipped + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Stop] + end + it 'fabricates a core skipped status' do expect(status).to be_a Gitlab::Ci::Status::Build::Stop end -- cgit v1.2.1 From 528c06882560eb14d14babf5cf5bb73d2276cb0c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:26:21 +0100 Subject: Fix a Rubocop offense in detailed status factory --- spec/lib/gitlab/ci/status/factory_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/ci/status/factory_spec.rb b/spec/lib/gitlab/ci/status/factory_spec.rb index f1b758640a7..bbf9c7c83a3 100644 --- a/spec/lib/gitlab/ci/status/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/factory_spec.rb @@ -11,7 +11,7 @@ describe Gitlab::Ci::Status::Factory do let(:resource) { double('resource', status: simple_status) } let(:expected_status) do - Gitlab::Ci::Status.const_get(simple_status.capitalize) + Gitlab::Ci::Status.const_get(simple_status.capitalize) end it "fabricates a core status #{simple_status}" do -- cgit v1.2.1 From ee18d89f3dc2b00f7e9514e21c12139ecc8f9224 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:46:53 +0100 Subject: Extend pipeline detailed status factory specs --- spec/lib/gitlab/ci/status/pipeline/factory_spec.rb | 45 ++++++++++++++-------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb index 0df6e881877..b10a447c27a 100644 --- a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb @@ -3,29 +3,32 @@ require 'spec_helper' describe Gitlab::Ci::Status::Pipeline::Factory do let(:user) { create(:user) } let(:project) { pipeline.project } - - subject do - described_class.new(pipeline, user) - end - - let(:status) do - subject.fabricate! - end + let(:status) { factory.fabricate! } + let(:factory) { described_class.new(pipeline, user) } before do project.team << [user, :developer] end context 'when pipeline has a core status' do - HasStatus::AVAILABLE_STATUSES.each do |core_status| - context "when core status is #{core_status}" do - let(:pipeline) do - create(:ci_pipeline, status: core_status) + HasStatus::AVAILABLE_STATUSES.each do |simple_status| + context "when core status is #{simple_status}" do + let(:pipeline) { create(:ci_pipeline, status: simple_status) } + + let(:expected_status) do + Gitlab::Ci::Status.const_get(simple_status.capitalize) + end + + it "matches correct core status for #{simple_status}" do + expect(factory.core_status).to be_a expected_status end - it "fabricates a core status #{core_status}" do - expect(status).to be_a( - Gitlab::Ci::Status.const_get(core_status.capitalize)) + it 'does not matche extended statuses' do + expect(factory.extended_statuses).to be_empty + end + + it "fabricates a core status #{simple_status}" do + expect(status).to be_a expected_status end it 'extends core status with common pipeline methods' do @@ -47,9 +50,17 @@ describe Gitlab::Ci::Status::Pipeline::Factory do create(:ci_build, :allowed_to_fail, :failed, pipeline: pipeline) end + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Success + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::SuccessWarning] + end + it 'fabricates extended "success with warnings" status' do - expect(status) - .to be_a Gitlab::Ci::Status::SuccessWarning + expect(status).to be_a Gitlab::Ci::Status::SuccessWarning end it 'extends core status with common pipeline method' do -- cgit v1.2.1 From cb316804aa59986c2a31bd8bcbc056e7fc0e66c0 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Thu, 12 Jan 2017 11:27:22 -0500 Subject: Add hover state to MR comment reply button --- app/assets/stylesheets/framework/buttons.scss | 2 +- changelogs/unreleased/input-button-hover.yml | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/input-button-hover.yml diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index e04a87a7327..bb6129158d9 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -324,7 +324,7 @@ &:focus { cursor: text; box-shadow: none; - border-color: $border-color; + border-color: lighten($dropdown-input-focus-border, 20%); color: $gray-darkest; background-color: $gray-light; } diff --git a/changelogs/unreleased/input-button-hover.yml b/changelogs/unreleased/input-button-hover.yml new file mode 100644 index 00000000000..cbb35adb769 --- /dev/null +++ b/changelogs/unreleased/input-button-hover.yml @@ -0,0 +1,4 @@ +--- +title: Add hover state to MR comment reply button +merge_request: +author: -- cgit v1.2.1 From 047107a4ab7d04efa8c30367d39ec896685dcd43 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Thu, 12 Jan 2017 12:17:14 -0500 Subject: Add margin to math blocks --- app/assets/stylesheets/framework/typography.scss | 2 +- changelogs/unreleased/26472-math-margin.yml | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/26472-math-margin.yml diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index bd58a26f429..54958973f15 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -10,7 +10,7 @@ max-width: 100%; } - *:first-child { + *:first-child:not(.katex-display) { margin-top: 0; } diff --git a/changelogs/unreleased/26472-math-margin.yml b/changelogs/unreleased/26472-math-margin.yml new file mode 100644 index 00000000000..3999f521558 --- /dev/null +++ b/changelogs/unreleased/26472-math-margin.yml @@ -0,0 +1,4 @@ +--- +title: Add margin to markdown math blocks +merge_request: +author: -- cgit v1.2.1 From 2f0bbd3d1dcc24ae7cf3fdbaa0b8db7f60e365ba Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 5 Jan 2017 10:41:15 +0000 Subject: Adds .catch to the request in order to handle erros and show a feedback to the user --- app/assets/javascripts/environments/components/environment.js.es6 | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/environments/components/environment.js.es6 b/app/assets/javascripts/environments/components/environment.js.es6 index 8b6fafb6104..80384ea8d72 100644 --- a/app/assets/javascripts/environments/components/environment.js.es6 +++ b/app/assets/javascripts/environments/components/environment.js.es6 @@ -1,6 +1,7 @@ -/* eslint-disable no-param-reassign */ +/* eslint-disable no-param-reassign, no-new */ /* global Vue */ /* global EnvironmentsService */ +/* global Flash */ //= require vue //= require vue-resource @@ -121,6 +122,10 @@ .then((json) => { this.store.storeEnvironments(json); this.isLoading = false; + }) + .catch(() => { + this.isLoading = false; + new Flash('An error occurred while fetching the environments.', 'alert'); }); }, -- cgit v1.2.1 From 463fddeafc945e4be249ba74ed510190ff9cedb6 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 5 Jan 2017 14:51:29 +0000 Subject: Adds tests Adds changelog entry Finishes tests Fix eslint errors Fix teaspoon test timing out --- .../environments/components/environment.js.es6 | 4 +- .../environments/stores/environments_store.js.es6 | 4 +- .../25507-handle-errors-environment-list.yml | 4 + .../environments/environment_spec.js.es6 | 127 +++++++++++++++++++++ spec/javascripts/environments/mock_data.js.es6 | 14 +++ .../fixtures/environments/environments.html.haml | 2 +- 6 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/25507-handle-errors-environment-list.yml create mode 100644 spec/javascripts/environments/environment_spec.js.es6 diff --git a/app/assets/javascripts/environments/components/environment.js.es6 b/app/assets/javascripts/environments/components/environment.js.es6 index 80384ea8d72..6e450df3c41 100644 --- a/app/assets/javascripts/environments/components/environment.js.es6 +++ b/app/assets/javascripts/environments/components/environment.js.es6 @@ -193,7 +193,7 @@
-

+

You don't have any environments right now.

@@ -207,7 +207,7 @@ + class="btn btn-create js-new-environment-button"> New Environment

diff --git a/app/assets/javascripts/environments/stores/environments_store.js.es6 b/app/assets/javascripts/environments/stores/environments_store.js.es6 index 0204a903ab5..46e39a15ac9 100644 --- a/app/assets/javascripts/environments/stores/environments_store.js.es6 +++ b/app/assets/javascripts/environments/stores/environments_store.js.es6 @@ -59,7 +59,7 @@ if (occurs.length) { acc[acc.indexOf(occurs[0])].children.push(environment); - acc[acc.indexOf(occurs[0])].children.sort(this.sortByName); + acc[acc.indexOf(occurs[0])].children.slice().sort(this.sortByName); } else { acc.push({ name: environment.environment_type, @@ -73,7 +73,7 @@ } return acc; - }, []).sort(this.sortByName); + }, []).slice().sort(this.sortByName); this.state.environments = environmentsTree; diff --git a/changelogs/unreleased/25507-handle-errors-environment-list.yml b/changelogs/unreleased/25507-handle-errors-environment-list.yml new file mode 100644 index 00000000000..4e9794f7917 --- /dev/null +++ b/changelogs/unreleased/25507-handle-errors-environment-list.yml @@ -0,0 +1,4 @@ +--- +title: Handle HTTP errors in environment list +merge_request: +author: diff --git a/spec/javascripts/environments/environment_spec.js.es6 b/spec/javascripts/environments/environment_spec.js.es6 new file mode 100644 index 00000000000..20e11ca3738 --- /dev/null +++ b/spec/javascripts/environments/environment_spec.js.es6 @@ -0,0 +1,127 @@ +/* global Vue, environment */ + +//= require vue +//= require vue-resource +//= require flash +//= require environments/stores/environments_store +//= require environments/components/environment +//= require ./mock_data + +describe('Environment', () => { + preloadFixtures('environments/environments'); + + let component; + + beforeEach(() => { + loadFixtures('environments/environments'); + }); + + describe('successfull request', () => { + describe('without environments', () => { + const environmentsEmptyResponseInterceptor = (request, next) => { + next(request.respondWith(JSON.stringify([]), { + status: 200, + })); + }; + + beforeEach(() => { + Vue.http.interceptors.push(environmentsEmptyResponseInterceptor); + }); + + afterEach(() => { + Vue.http.interceptors = _.without( + Vue.http.interceptors, environmentsEmptyResponseInterceptor, + ); + }); + + it('should render the empty state', (done) => { + component = new gl.environmentsList.EnvironmentsComponent({ + el: document.querySelector('#environments-list-view'), + propsData: { + store: gl.environmentsList.EnvironmentsStore.create(), + }, + }); + + setTimeout(() => { + expect( + component.$el.querySelector('.js-new-environment-button').textContent, + ).toContain('New Environment'); + + expect( + component.$el.querySelector('.js-blank-state-title').textContent, + ).toContain('You don\'t have any environments right now.'); + + done(); + }, 0); + }); + }); + + describe('with environments', () => { + const environmentsResponseInterceptor = (request, next) => { + next(request.respondWith(JSON.stringify([environment]), { + status: 200, + })); + }; + + beforeEach(() => { + Vue.http.interceptors.push(environmentsResponseInterceptor); + }); + + afterEach(() => { + Vue.http.interceptors = _.without( + Vue.http.interceptors, environmentsResponseInterceptor, + ); + }); + + it('should render a table with environments', (done) => { + component = new gl.environmentsList.EnvironmentsComponent({ + el: document.querySelector('#environments-list-view'), + propsData: { + store: gl.environmentsList.EnvironmentsStore.create(), + }, + }); + + setTimeout(() => { + expect( + component.$el.querySelectorAll('table tbody tr').length, + ).toEqual(1); + done(); + }, 0); + }); + }); + }); + + describe('unsuccessfull request', () => { + const environmentsErrorResponseInterceptor = (request, next) => { + next(request.respondWith(JSON.stringify([]), { + status: 500, + })); + }; + + beforeEach(() => { + Vue.http.interceptors.push(environmentsErrorResponseInterceptor); + }); + + afterEach(() => { + Vue.http.interceptors = _.without( + Vue.http.interceptors, environmentsErrorResponseInterceptor, + ); + }); + + it('should render empty state', (done) => { + component = new gl.environmentsList.EnvironmentsComponent({ + el: document.querySelector('#environments-list-view'), + propsData: { + store: gl.environmentsList.EnvironmentsStore.create(), + }, + }); + + setTimeout(() => { + expect( + component.$el.querySelector('.js-blank-state-title').textContent, + ).toContain('You don\'t have any environments right now.'); + done(); + }, 0); + }); + }); +}); diff --git a/spec/javascripts/environments/mock_data.js.es6 b/spec/javascripts/environments/mock_data.js.es6 index 9e16bc3e6a5..8ecd01f9a83 100644 --- a/spec/javascripts/environments/mock_data.js.es6 +++ b/spec/javascripts/environments/mock_data.js.es6 @@ -133,3 +133,17 @@ const environmentsList = [ updated_at: '2016-11-07T11:11:16.525Z', }, ]; + +const environment = { + id: 4, + name: 'production', + state: 'available', + external_url: 'http://production.', + environment_type: null, + last_deployment: {}, + 'stoppable?': false, + environment_path: '/root/review-app/environments/4', + stop_path: '/root/review-app/environments/4/stop', + created_at: '2016-12-16T11:51:04.690Z', + updated_at: '2016-12-16T12:04:51.133Z', +}; diff --git a/spec/javascripts/fixtures/environments/environments.html.haml b/spec/javascripts/fixtures/environments/environments.html.haml index d89bc50c1f0..e6000fbb553 100644 --- a/spec/javascripts/fixtures/environments/environments.html.haml +++ b/spec/javascripts/fixtures/environments/environments.html.haml @@ -1,5 +1,5 @@ %div - #environments-list-view{ data: { environments_data: "https://gitlab.com/foo/environments", + #environments-list-view{ data: { environments_data: "foo/environments", "can-create-deployment" => "true", "can-read-environment" => "true", "can-create-environment" => "true", -- cgit v1.2.1 From 7ffd0d3be83caecdec54edfacaa31971f885079f Mon Sep 17 00:00:00 2001 From: Guillaume Simon Date: Mon, 9 Jan 2017 19:37:58 +0100 Subject: Gitlab from sources : proper utf8mb4 support for MySQL 5.5 to 5.7 --- config/database.yml.mysql | 8 +- doc/install/database_mysql.md | 205 +++++++++++++++++++++++++++++++++++++++++- doc/update/8.15-to-8.16.md | 2 + 3 files changed, 208 insertions(+), 7 deletions(-) diff --git a/config/database.yml.mysql b/config/database.yml.mysql index d9702870249..a33e40e8eb3 100644 --- a/config/database.yml.mysql +++ b/config/database.yml.mysql @@ -3,8 +3,8 @@ # production: adapter: mysql2 - encoding: utf8mb4 - collation: utf8mb4_general_ci + encoding: utf8 + collation: utf8_general_ci reconnect: false database: gitlabhq_production pool: 10 @@ -18,8 +18,8 @@ production: # development: adapter: mysql2 - encoding: utf8mb4 - collation: utf8mb4_general_ci + encoding: utf8 + collation: utf8_general_ci reconnect: false database: gitlabhq_development pool: 5 diff --git a/doc/install/database_mysql.md b/doc/install/database_mysql.md index 322680f0cf4..65bfb0f7d6e 100644 --- a/doc/install/database_mysql.md +++ b/doc/install/database_mysql.md @@ -4,7 +4,7 @@ We do not recommend using MySQL due to various issues. For example, case [(in)sensitivity](https://dev.mysql.com/doc/refman/5.0/en/case-sensitivity.html) and [problems](https://bugs.mysql.com/bug.php?id=65830) that [suggested](https://bugs.mysql.com/bug.php?id=50909) [fixes](https://bugs.mysql.com/bug.php?id=65830) [have](https://bugs.mysql.com/bug.php?id=63164). -## MySQL +## Initial database setup # Install the database packages sudo apt-get install -y mysql-server mysql-client libmysqlclient-dev @@ -32,8 +32,11 @@ We do not recommend using MySQL due to various issues. For example, case [(in)se # If this fails, check your MySQL config files (e.g. `/etc/mysql/*.cnf`, `/etc/mysql/conf.d/*`) for the setting "innodb = off" mysql> SET storage_engine=INNODB; + # If you have MySQL < 5.7.7 and want to enable utf8mb4 character set support with your GitLab install, you must set the following NOW: + mysql> SET GLOBAL innodb_file_per_table=1, innodb_file_format=Barracuda, innodb_large_prefix=1; + # Create the GitLab production database - mysql> CREATE DATABASE IF NOT EXISTS `gitlabhq_production` DEFAULT CHARACTER SET `utf8` COLLATE `utf8_unicode_ci`; + mysql> CREATE DATABASE IF NOT EXISTS `gitlabhq_production` DEFAULT CHARACTER SET `utf8` COLLATE `utf8_general_ci`; # Grant the GitLab user necessary permissions on the database mysql> GRANT SELECT, INSERT, UPDATE, DELETE, CREATE, CREATE TEMPORARY TABLES, DROP, INDEX, ALTER, LOCK TABLES, REFERENCES ON `gitlabhq_production`.* TO 'git'@'localhost'; @@ -51,7 +54,203 @@ We do not recommend using MySQL due to various issues. For example, case [(in)se # Quit the database session mysql> \q - # You are done installing the database and can go back to the rest of the installation. + # You are done installing the database for now and can go back to the rest of the installation. + +Please proceed to the rest of the installation before running through the utf8mb4 support section. + + +### MySQL utf8mb4 support + +After installation or upgrade, remember to [convert any new tables](#convert) to `utf8mb4`/`utf8mb4_general_ci`. + +--- + +GitLab 8.14 has introduced [a feature](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7420) requiring `utf8mb4` encoding to be supported in your GitLab MySQL Database, which is not the case if you have setup your database before GitLab 8.16. + +Follow the below instructions to ensure you use the most up to date requirements for your GitLab MySQL Database. + +**We are about to do the following:** +- Ensure you can enable `utf8mb4` encoding and `utf8mb4_general_ci` collation for your GitLab DB, tables and data. +- Convert your GitLab tables and data from `utf8`/`utf8_general_ci` to `utf8mb4`/`utf8mb4_general_ci` + +### Check for utf8mb4 support + +#### Check for InnoDB File-Per-Table Tablespaces + +We need to check, enable and maybe convert your existing GitLab DB tables to the [InnoDB File-Per-Table Tablespaces](http://dev.mysql.com/doc/refman/5.7/en/innodb-multiple-tablespaces.html) as a prerequise for supporting **utfb8mb4 with long indexes** required by recent GitLab databases. + + # Login to MySQL + mysql -u root -p + + # Type the MySQL root password + mysql > use gitlabhq_production; + + # Check your MySQL version is >= 5.5.3 (GitLab requires 5.5.14+) + mysql > SHOW VARIABLES LIKE 'version'; + +---------------+-----------------+ + | Variable_name | Value | + +---------------+-----------------+ + | version | 5.5.53-0+deb8u1 | + +---------------+-----------------+ + + # Note where is your MySQL data dir for later: + mysql > select @@datadir; + +----------------+ + | @@datadir | + +----------------+ + | /var/lib/mysql | + +----------------+ + + # Note whether your MySQL server runs with innodb_file_per_table ON or OFF: + mysql> SELECT @@innodb_file_per_table; + +-------------------------+ + | @@innodb_file_per_table | + +-------------------------+ + | 1 | + +-------------------------+ + + # You can now quit the database session + mysql> \q + +> You need **MySQL 5.5.3 or later** to perform this update. + +Whatever the results of your checks above, we now need to check if your GitLab database has been created using [InnoDB File-Per-Table Tablespaces](http://dev.mysql.com/doc/refman/5.7/en/innodb-multiple-tablespaces.html) (i.e. `innodb_file_per_table` was set to **1** at initial setup time). + +> Note: This setting is [enabled by default](http://dev.mysql.com/doc/refman/5.7/en/innodb-parameters.html#sysvar_innodb_file_per_table) since MySQL 5.6.6. + + # Run this command with root privileges, replace the data dir if different: + sudo ls -lh /var/lib/mysql/gitlabhq_production/*.ibd | wc -l + + # Run this command with root privileges, replace the data dir if different: + sudo ls -lh /var/lib/mysql/gitlabhq_production/*.frm | wc -l + + +- **Case 1: a result > 0 for both commands** + +Congrats, your GitLab database uses the right InnoDB tablespace format. + +However, you must still ensure that any **future tables** created by GitLab will still use the right format: + +- If `SELECT @@innodb_file_per_table` returned **1** previously, your server is running correctly. +> It's however a requirement to check *now* that this setting is indeed persisted in your [my.cnf](https://dev.mysql.com/doc/refman/5.7/en/tablespace-enabling.html) file! + +- If `SELECT @@innodb_file_per_table` returned **0** previously, your server is not running correctly. +> [Enable innodb_file_per_table](https://dev.mysql.com/doc/refman/5.7/en/tablespace-enabling.html) by running in a MySQL session as root the command `SET GLOBAL innodb_file_per_table=1, innodb_file_format=Barracuda;` and persist the two settings in your [my.cnf](https://dev.mysql.com/doc/refman/5.7/en/tablespace-enabling.html) file + +Now, if you have a **different result** returned by the 2 commands above, it means you have a **mix of tables format** uses in your GitLab database. This can happen if your MySQL server had different values for `innodb_file_per_table` in its life and you updated GitLab at different moments with those inconsistent values. So keep reading. + +- **Case 2: a result equals to "0" OR not the same result for both commands** + +Unfortunately, none or only some of your GitLab database tables use the GitLab requirement of [InnoDB File-Per-Table Tablespaces](http://dev.mysql.com/doc/refman/5.7/en/innodb-multiple-tablespaces.html). + +Let's enable what we need on the running server: + + # Login to MySQL + mysql -u root -p + + # Type the MySQL root password + + # Enable innodb_file_per_table and set innodb_file_format on the running server: + mysql > SET GLOBAL innodb_file_per_table=1, innodb_file_format=Barracuda; + + # You can now quit the database session + mysql> \q + +> Now, **persist** [innodb_file_per_table](https://dev.mysql.com/doc/refman/5.6/en/tablespace-enabling.html) and [innodb_file_format](https://dev.mysql.com/doc/refman/5.6/en/innodb-file-format-enabling.html) in your `my.cnf` file. + +Ensure at this stage that your GitLab instance is indeed **stopped**. + +Now, let's convert all the GitLab database tables to the new tablespace format: + + # Login to MySQL + mysql -u root -p + + # Type the MySQL root password + mysql > use gitlabhq_production; + + # Safety check: you should still have those values set as follow: + mysql> SELECT @@innodb_file_per_table, @@innodb_file_format; + +-------------------------+----------------------+ + | @@innodb_file_per_table | @@innodb_file_format | + +-------------------------+----------------------+ + | 1 | Barracuda | + +-------------------------+----------------------+ + + mysql > SELECT CONCAT('ALTER TABLE `', TABLE_NAME,'` ENGINE=InnoDB;') AS 'Copy & run these SQL statements:' FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA="gitlabhq_production" AND TABLE_TYPE="BASE TABLE"; + + # If previous query returned results, copy & run all shown SQL statements + + # You can now quit the database session + mysql> \q + +--- + +#### Check for proper InnoDB File Format, Row Format, Large Prefix and tables conversion + +We need to check, enable and probably convert your existing GitLab DB tables to use the [Barracuda InnoDB file format](https://dev.mysql.com/doc/refman/5.6/en/innodb-file-format.html), the [DYNAMIC row format](https://dev.mysql.com/doc/refman/5.6/en/glossary.html#glos_dynamic_row_format) and [innodb_large_prefix](http://dev.mysql.com/doc/refman/5.7/en/innodb-parameters.html#sysvar_innodb_large_prefix) as a second prerequisite for supporting **utfb8mb4 with long indexes** used by recent GitLab databases. + + # Login to MySQL + mysql -u root -p + + # Type the MySQL root password + mysql > use gitlabhq_production; + + # Set innodb_file_format and innodb_large_prefix on the running server: + # Note: These are the default settings only for MySQL 5.7.7 and later. + + mysql > SET GLOBAL innodb_file_format=Barracuda, innodb_large_prefix=1; + + # Your DB must be (still) using utf8/utf8_general_ci as default encoding and collation. + # We will NOT change the default encoding and collation on the DB in order to support future GitLab migrations creating tables + # that require "long indexes support" on installations using MySQL <= 5.7.9. + # However, when such migrations occur, you will have to follow this guide again to convert the newly created tables to the proper encoding/collation. + + # This should return the following: + mysql> SELECT @@character_set_database, @@collation_database; + +--------------------------+----------------------+ + | @@character_set_database | @@collation_database | + +--------------------------+----------------------+ + | utf8 | utf8_general_ci | + +--------------------------+----------------------+ + +> Now, ensure that [innodb_file_format](https://dev.mysql.com/doc/refman/5.6/en/tablespace-enabling.html) and [innodb_large_prefix](http://dev.mysql.com/doc/refman/5.7/en/innodb-parameters.html#sysvar_innodb_large_prefix) are **persisted** in your `my.cnf` file. + +#### Tables and data conversion to utf8mb4 + + +Now that you have a persistent MySQL setup, you can safely upgrade tables after setup or upgrade time: + + # Convert tables not using ROW_FORMAT DYNAMIC: + + mysql> SELECT CONCAT('ALTER TABLE `', TABLE_NAME,'` ROW_FORMAT=DYNAMIC;') AS 'Copy & run these SQL statements:' FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA="gitlabhq_production" AND TABLE_TYPE="BASE TABLE" AND ROW_FORMAT!="Dynamic"; + + # !! If previous query returned results, copy & run all shown SQL statements + + # Convert tables/columns not using utf8mb4/utf8mb4_general_ci as encoding/collation: + + mysql > SET foreign_key_checks = 0; + + mysql > SELECT CONCAT('ALTER TABLE `', TABLE_NAME,'` CONVERT TO CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci;') AS 'Copy & run these SQL statements:' FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA="gitlabhq_production" AND TABLE_COLLATION != "utf8mb4_general_ci" AND TABLE_TYPE="BASE TABLE"; + + # !! If previous query returned results, copy & run all shown SQL statements + + # Turn foreign key checks back on + mysql > SET foreign_key_checks = 1; + + # You can now quit the database session + mysql> \q + +Ensure your GitLab database configuration file uses a proper connection encoding and collation: + +```sudo -u git -H editor config/database.yml``` + + production: + adapter: mysql2 + encoding: utf8mb4 + collation: utf8mb4_general_ci + +[Restart your GitLab instance](../administration/restart_gitlab.md). + ## MySQL strings limits diff --git a/doc/update/8.15-to-8.16.md b/doc/update/8.15-to-8.16.md index 3d68fe201a7..d494db76912 100644 --- a/doc/update/8.15-to-8.16.md +++ b/doc/update/8.15-to-8.16.md @@ -97,6 +97,8 @@ sudo -u git -H bundle exec rake db:migrate RAILS_ENV=production sudo -u git -H bundle exec rake assets:clean assets:precompile cache:clear RAILS_ENV=production ``` +**MySQL installations**: Run through the `MySQL strings limits` and `Tables and data conversion to utf8mb4` [tasks](../install/database_mysql.md). + ### 6. Update gitlab-workhorse Install and compile gitlab-workhorse. This requires -- cgit v1.2.1 From d92f1abafe790e2a14161b1f096e39e0dd6e777b Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 13 Jan 2017 08:54:20 -0500 Subject: Fixed some HAML errors on milestone#show The issuable reference wasn't correctly rendered on the page 2 counts where rendered in the top bar of the panel --- app/views/shared/milestones/_issuable.html.haml | 2 +- app/views/shared/milestones/_issuables.html.haml | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/views/shared/milestones/_issuable.html.haml b/app/views/shared/milestones/_issuable.html.haml index 51c195ffbcd..28935c8b598 100644 --- a/app/views/shared/milestones/_issuable.html.haml +++ b/app/views/shared/milestones/_issuable.html.haml @@ -16,7 +16,7 @@ = link_to_gfm issuable.title, [project.namespace.becomes(Namespace), project, issuable], title: issuable.title .issuable-detail = link_to [project.namespace.becomes(Namespace), project, issuable] do - %span.issuable-number >= issuable.to_reference + %span.issuable-number= issuable.to_reference - issuable.labels.each do |label| = link_to polymorphic_path(base_url_args, { milestone_title: @milestone.title, label_name: label.title, state: 'all' }) do diff --git a/app/views/shared/milestones/_issuables.html.haml b/app/views/shared/milestones/_issuables.html.haml index c8fd45c4319..d7eed9e7418 100644 --- a/app/views/shared/milestones/_issuables.html.haml +++ b/app/views/shared/milestones/_issuables.html.haml @@ -7,8 +7,6 @@ .left = title - if show_counter - .right - = issuables.size .pull-right= number_with_delimiter(issuables.size) - class_prefix = dom_class(issuables).pluralize -- cgit v1.2.1 From 51c4b20c48f29fe34fd1306f7a115f645eb9fb71 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 5 Jan 2017 14:43:50 +0200 Subject: Refactor Namespace code related to nested groups Signed-off-by: Dmitriy Zaporozhets --- app/helpers/groups_helper.rb | 2 +- app/models/group.rb | 2 +- app/models/namespace.rb | 22 ++++++++++++++++++++-- app/models/route.rb | 4 ++-- spec/models/namespace_spec.rb | 28 ++++++++++++++++++++++------ spec/models/route_spec.rb | 2 +- 6 files changed, 47 insertions(+), 13 deletions(-) diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index 77dc9e7d538..926c9703628 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -14,7 +14,7 @@ module GroupsHelper def group_title(group, name = nil, url = nil) full_title = '' - group.parents.each do |parent| + group.ancestors.each do |parent| full_title += link_to(simple_sanitize(parent.name), group_path(parent)) full_title += ' / '.html_safe end diff --git a/app/models/group.rb b/app/models/group.rb index 99675ddb366..4cdfd022094 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -201,7 +201,7 @@ class Group < Namespace end def members_with_parents - GroupMember.where(requested_at: nil, source_id: parents.map(&:id).push(id)) + GroupMember.where(requested_at: nil, source_id: ancestors.map(&:id).push(id)) end def users_with_parents diff --git a/app/models/namespace.rb b/app/models/namespace.rb index d41833de66f..d3a4ddbb3bf 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -183,8 +183,26 @@ class Namespace < ActiveRecord::Base end end - def parents - @parents ||= parent ? parent.parents + [parent] : [] + # Scopes the model on ancestors of the record + def ancestors + if parent_id + path = route.path + paths = [] + + until path.blank? + path = path.rpartition('/').first + paths << path + end + + self.class.joins(:route).where('routes.path IN (?)', paths).order_id_asc + else + self.class.none + end + end + + # Scopes the model on direct and indirect children of the record + def descendants + self.class.joins(:route).where('routes.path LIKE ?', "#{route.path}/%").order_id_asc end private diff --git a/app/models/route.rb b/app/models/route.rb index caf596efa79..ebd18dce737 100644 --- a/app/models/route.rb +++ b/app/models/route.rb @@ -8,9 +8,9 @@ class Route < ActiveRecord::Base presence: true, uniqueness: { case_sensitive: false } - after_update :rename_children, if: :path_changed? + after_update :rename_descendants, if: :path_changed? - def rename_children + def rename_descendants # We update each row separately because MySQL does not have regexp_replace. # rubocop:disable Rails/FindEach Route.where('path LIKE ?', "#{path_was}/%").each do |route| diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 600538ff5f4..42e8a48abe9 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -5,6 +5,8 @@ describe Namespace, models: true do it { is_expected.to have_many :projects } it { is_expected.to have_many :project_statistics } + it { is_expected.to belong_to :parent } + it { is_expected.to have_many :children } it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_uniqueness_of(:name).scoped_to(:parent_id) } @@ -182,17 +184,31 @@ describe Namespace, models: true do it { expect(nested_group.full_name).to eq("#{group.name} / #{nested_group.name}") } end - describe '#parents' do + describe '#ancestors' do let(:group) { create(:group) } let(:nested_group) { create(:group, parent: group) } let(:deep_nested_group) { create(:group, parent: nested_group) } let(:very_deep_nested_group) { create(:group, parent: deep_nested_group) } - it 'returns the correct parents' do - expect(very_deep_nested_group.parents).to eq([group, nested_group, deep_nested_group]) - expect(deep_nested_group.parents).to eq([group, nested_group]) - expect(nested_group.parents).to eq([group]) - expect(group.parents).to eq([]) + it 'returns the correct ancestors' do + expect(very_deep_nested_group.ancestors).to eq([group, nested_group, deep_nested_group]) + expect(deep_nested_group.ancestors).to eq([group, nested_group]) + expect(nested_group.ancestors).to eq([group]) + expect(group.ancestors).to eq([]) + end + end + + describe '#descendants' do + let!(:group) { create(:group) } + let!(:nested_group) { create(:group, parent: group) } + let!(:deep_nested_group) { create(:group, parent: nested_group) } + let!(:very_deep_nested_group) { create(:group, parent: deep_nested_group) } + + it 'returns the correct descendants' do + expect(very_deep_nested_group.descendants.to_a).to eq([]) + expect(deep_nested_group.descendants.to_a).to eq([very_deep_nested_group]) + expect(nested_group.descendants.to_a).to eq([deep_nested_group, very_deep_nested_group]) + expect(group.descendants.to_a).to eq([nested_group, deep_nested_group, very_deep_nested_group]) end end end diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb index 8481a9bef16..dd2a5109abc 100644 --- a/spec/models/route_spec.rb +++ b/spec/models/route_spec.rb @@ -14,7 +14,7 @@ describe Route, models: true do it { is_expected.to validate_uniqueness_of(:path) } end - describe '#rename_children' do + describe '#rename_descendants' do let!(:nested_group) { create(:group, path: "test", parent: group) } let!(:deep_nested_group) { create(:group, path: "foo", parent: nested_group) } let!(:similar_group) { create(:group, path: 'gitlab-org') } -- cgit v1.2.1 From c3a940000ea20d6682313640e1a0fda9ff68dbdf Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Wed, 12 Oct 2016 19:07:36 +0200 Subject: Handles unsubscribe from notifications via email - allows unsubscription processing of email in format "reply+%{key}+unsubscribe@acme.com" (example) - if config.address includes %{key} and replies are enabled every unsubscriable message will include mailto: link in its List-Unsubscribe header --- app/mailers/notify.rb | 20 ++++--- ...ail-address-to-unsubscribe-list-header-in-email | 4 ++ lib/gitlab/email/handler.rb | 3 +- lib/gitlab/email/handler/base_handler.rb | 43 +-------------- lib/gitlab/email/handler/create_issue_handler.rb | 1 + lib/gitlab/email/handler/create_note_handler.rb | 7 ++- lib/gitlab/email/handler/reply_processing.rb | 54 +++++++++++++++++++ lib/gitlab/email/handler/unsubscribe_handler.rb | 32 ++++++++++++ lib/gitlab/incoming_email.rb | 9 +++- spec/lib/gitlab/email/email_shared_blocks.rb | 2 +- .../email/handler/create_issue_handler_spec.rb | 2 +- .../email/handler/create_note_handler_spec.rb | 2 +- .../email/handler/unsubscribe_handler_spec.rb | 61 ++++++++++++++++++++++ spec/lib/gitlab/incoming_email_spec.rb | 42 +++++++++++++++ spec/support/notify_shared_examples.rb | 17 +++++- 15 files changed, 243 insertions(+), 56 deletions(-) create mode 100644 changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email create mode 100644 lib/gitlab/email/handler/reply_processing.rb create mode 100644 lib/gitlab/email/handler/unsubscribe_handler.rb create mode 100644 spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 0bc1c19e9cd..0cd3456b4de 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -107,15 +107,11 @@ class Notify < BaseMailer def mail_thread(model, headers = {}) add_project_headers + add_unsubscription_headers_and_links + headers["X-GitLab-#{model.class.name}-ID"] = model.id headers['X-GitLab-Reply-Key'] = reply_key - if !@labels_url && @sent_notification && @sent_notification.unsubscribable? - headers['List-Unsubscribe'] = "<#{unsubscribe_sent_notification_url(@sent_notification, force: true)}>" - - @sent_notification_url = unsubscribe_sent_notification_url(@sent_notification) - end - if Gitlab::IncomingEmail.enabled? address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key)) address.display_name = @project.name_with_namespace @@ -171,4 +167,16 @@ class Notify < BaseMailer headers['X-GitLab-Project-Id'] = @project.id headers['X-GitLab-Project-Path'] = @project.path_with_namespace end + + def add_unsubscription_headers_and_links + return unless !@labels_url && @sent_notification && @sent_notification.unsubscribable? + + list_unsubscribe_methods = [unsubscribe_sent_notification_url(@sent_notification, force: true)] + if Gitlab::IncomingEmail.enabled? && Gitlab::IncomingEmail.supports_wildcard? + list_unsubscribe_methods << "mailto:#{Gitlab::IncomingEmail.unsubscribe_address(reply_key)}" + end + + headers['List-Unsubscribe'] = list_unsubscribe_methods.map { |e| "<#{e}>" }.join(',') + @sent_notification_url = unsubscribe_sent_notification_url(@sent_notification) + end end diff --git a/changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email b/changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email new file mode 100644 index 00000000000..f4011b756a5 --- /dev/null +++ b/changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email @@ -0,0 +1,4 @@ +--- +title: Handle unsubscribe from email notifications via replying to reply+%{key}+unsubscribe@ address +merge_request: 6597 +author: diff --git a/lib/gitlab/email/handler.rb b/lib/gitlab/email/handler.rb index bd3267e2a80..bd2f5d3615e 100644 --- a/lib/gitlab/email/handler.rb +++ b/lib/gitlab/email/handler.rb @@ -1,10 +1,11 @@ require 'gitlab/email/handler/create_note_handler' require 'gitlab/email/handler/create_issue_handler' +require 'gitlab/email/handler/unsubscribe_handler' module Gitlab module Email module Handler - HANDLERS = [CreateNoteHandler, CreateIssueHandler] + HANDLERS = [UnsubscribeHandler, CreateNoteHandler, CreateIssueHandler] def self.for(mail, mail_key) HANDLERS.find do |klass| diff --git a/lib/gitlab/email/handler/base_handler.rb b/lib/gitlab/email/handler/base_handler.rb index 7cccf465334..3f6ace0311a 100644 --- a/lib/gitlab/email/handler/base_handler.rb +++ b/lib/gitlab/email/handler/base_handler.rb @@ -9,52 +9,13 @@ module Gitlab @mail_key = mail_key end - def message - @message ||= process_message - end - - def author + def can_execute? raise NotImplementedError end - def project + def execute raise NotImplementedError end - - private - - def validate_permission!(permission) - raise UserNotFoundError unless author - raise UserBlockedError if author.blocked? - raise ProjectNotFound unless author.can?(:read_project, project) - raise UserNotAuthorizedError unless author.can?(permission, project) - end - - def process_message - message = ReplyParser.new(mail).execute.strip - add_attachments(message) - end - - def add_attachments(reply) - attachments = Email::AttachmentUploader.new(mail).execute(project) - - reply + attachments.map do |link| - "\n\n#{link[:markdown]}" - end.join - end - - def verify_record!(record:, invalid_exception:, record_name:) - return if record.persisted? - return if record.errors.key?(:commands_only) - - error_title = "The #{record_name} could not be created for the following reasons:" - - msg = error_title + record.errors.full_messages.map do |error| - "\n\n- #{error}" - end.join - - raise invalid_exception, msg - end end end end diff --git a/lib/gitlab/email/handler/create_issue_handler.rb b/lib/gitlab/email/handler/create_issue_handler.rb index 9f90a3ec2b2..127fae159d5 100644 --- a/lib/gitlab/email/handler/create_issue_handler.rb +++ b/lib/gitlab/email/handler/create_issue_handler.rb @@ -5,6 +5,7 @@ module Gitlab module Email module Handler class CreateIssueHandler < BaseHandler + include ReplyProcessing attr_reader :project_path, :incoming_email_token def initialize(mail, mail_key) diff --git a/lib/gitlab/email/handler/create_note_handler.rb b/lib/gitlab/email/handler/create_note_handler.rb index 447c7a6a6b9..d87ba427f4b 100644 --- a/lib/gitlab/email/handler/create_note_handler.rb +++ b/lib/gitlab/email/handler/create_note_handler.rb @@ -1,10 +1,13 @@ require 'gitlab/email/handler/base_handler' +require 'gitlab/email/handler/reply_processing' module Gitlab module Email module Handler class CreateNoteHandler < BaseHandler + include ReplyProcessing + def can_handle? mail_key =~ /\A\w+\z/ end @@ -24,6 +27,8 @@ module Gitlab record_name: 'comment') end + private + def author sent_notification.recipient end @@ -36,8 +41,6 @@ module Gitlab @sent_notification ||= SentNotification.for(mail_key) end - private - def create_note Notes::CreateService.new( project, diff --git a/lib/gitlab/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb new file mode 100644 index 00000000000..32c5caf93e8 --- /dev/null +++ b/lib/gitlab/email/handler/reply_processing.rb @@ -0,0 +1,54 @@ +module Gitlab + module Email + module Handler + module ReplyProcessing + private + + def author + raise NotImplementedError + end + + def project + raise NotImplementedError + end + + def message + @message ||= process_message + end + + def process_message + message = ReplyParser.new(mail).execute.strip + add_attachments(message) + end + + def add_attachments(reply) + attachments = Email::AttachmentUploader.new(mail).execute(project) + + reply + attachments.map do |link| + "\n\n#{link[:markdown]}" + end.join + end + + def validate_permission!(permission) + raise UserNotFoundError unless author + raise UserBlockedError if author.blocked? + raise ProjectNotFound unless author.can?(:read_project, project) + raise UserNotAuthorizedError unless author.can?(permission, project) + end + + def verify_record!(record:, invalid_exception:, record_name:) + return if record.persisted? + return if record.errors.key?(:commands_only) + + error_title = "The #{record_name} could not be created for the following reasons:" + + msg = error_title + record.errors.full_messages.map do |error| + "\n\n- #{error}" + end.join + + raise invalid_exception, msg + end + end + end + end +end diff --git a/lib/gitlab/email/handler/unsubscribe_handler.rb b/lib/gitlab/email/handler/unsubscribe_handler.rb new file mode 100644 index 00000000000..97d7a8d65ff --- /dev/null +++ b/lib/gitlab/email/handler/unsubscribe_handler.rb @@ -0,0 +1,32 @@ +require 'gitlab/email/handler/base_handler' + +module Gitlab + module Email + module Handler + class UnsubscribeHandler < BaseHandler + def can_handle? + mail_key =~ /\A\w+#{Regexp.escape(Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX)}\z/ + end + + def execute + raise SentNotificationNotFoundError unless sent_notification + return unless sent_notification.unsubscribable? + + noteable = sent_notification.noteable + raise NoteableNotFoundError unless noteable + noteable.unsubscribe(sent_notification.recipient) + end + + private + + def sent_notification + @sent_notification ||= SentNotification.for(reply_key) + end + + def reply_key + mail_key.sub(Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX, '') + end + end + end + end +end diff --git a/lib/gitlab/incoming_email.rb b/lib/gitlab/incoming_email.rb index 801dfde9a36..b91012d6405 100644 --- a/lib/gitlab/incoming_email.rb +++ b/lib/gitlab/incoming_email.rb @@ -1,5 +1,6 @@ module Gitlab module IncomingEmail + UNSUBSCRIBE_SUFFIX = '+unsubscribe'.freeze WILDCARD_PLACEHOLDER = '%{key}'.freeze class << self @@ -18,7 +19,11 @@ module Gitlab end def reply_address(key) - config.address.gsub(WILDCARD_PLACEHOLDER, key) + config.address.sub(WILDCARD_PLACEHOLDER, key) + end + + def unsubscribe_address(key) + config.address.sub(WILDCARD_PLACEHOLDER, "#{key}#{UNSUBSCRIBE_SUFFIX}") end def key_from_address(address) @@ -49,7 +54,7 @@ module Gitlab return nil unless wildcard_address regex = Regexp.escape(wildcard_address) - regex = regex.gsub(Regexp.escape('%{key}'), "(.+)") + regex = regex.sub(Regexp.escape(WILDCARD_PLACEHOLDER), '(.+)') Regexp.new(regex).freeze end end diff --git a/spec/lib/gitlab/email/email_shared_blocks.rb b/spec/lib/gitlab/email/email_shared_blocks.rb index 19298e261e3..9d806fc524d 100644 --- a/spec/lib/gitlab/email/email_shared_blocks.rb +++ b/spec/lib/gitlab/email/email_shared_blocks.rb @@ -18,7 +18,7 @@ shared_context :email_shared_context do end end -shared_examples :email_shared_examples do +shared_examples :reply_processing_shared_examples do context "when the user could not be found" do before do user.destroy diff --git a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb index cb3651e3845..08897a4c310 100644 --- a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb @@ -3,7 +3,7 @@ require_relative '../email_shared_blocks' describe Gitlab::Email::Handler::CreateIssueHandler, lib: true do include_context :email_shared_context - it_behaves_like :email_shared_examples + it_behaves_like :reply_processing_shared_examples before do stub_incoming_email_setting(enabled: true, address: "incoming+%{key}@appmail.adventuretime.ooo") diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb index 48660d1dd1b..cebbeff50cf 100644 --- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb @@ -3,7 +3,7 @@ require_relative '../email_shared_blocks' describe Gitlab::Email::Handler::CreateNoteHandler, lib: true do include_context :email_shared_context - it_behaves_like :email_shared_examples + it_behaves_like :reply_processing_shared_examples before do stub_incoming_email_setting(enabled: true, address: "reply+%{key}@appmail.adventuretime.ooo") diff --git a/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb b/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb new file mode 100644 index 00000000000..a444257754b --- /dev/null +++ b/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb @@ -0,0 +1,61 @@ +require 'spec_helper' +require_relative '../email_shared_blocks' + +describe Gitlab::Email::Handler::UnsubscribeHandler, lib: true do + include_context :email_shared_context + + before do + stub_incoming_email_setting(enabled: true, address: 'reply+%{key}@appmail.adventuretime.ooo') + stub_config_setting(host: 'localhost') + end + + let(:email_raw) { fixture_file('emails/valid_reply.eml').gsub(mail_key, "#{mail_key}+unsubscribe") } + let(:project) { create(:project, :public) } + let(:user) { create(:user) } + let(:noteable) { create(:issue, project: project) } + + let!(:sent_notification) { SentNotification.record(noteable, user.id, mail_key) } + + context 'when notification concerns a commit' do + let(:commit) { create(:commit, project: project) } + let!(:sent_notification) { SentNotification.record(commit, user.id, mail_key) } + + it 'handler does not raise an error' do + expect { receiver.execute }.not_to raise_error + end + end + + context 'user is unsubscribed' do + it 'leaves user unsubscribed' do + expect { receiver.execute }.not_to change { noteable.subscribed?(user) }.from(false) + end + end + + context 'user is subscribed' do + before do + noteable.subscribe(user) + end + + it 'unsubscribes user from notable' do + expect { receiver.execute }.to change { noteable.subscribed?(user) }.from(true).to(false) + end + end + + context 'when the noteable could not be found' do + before do + noteable.destroy + end + + it 'raises a NoteableNotFoundError' do + expect { receiver.execute }.to raise_error(Gitlab::Email::NoteableNotFoundError) + end + end + + context 'when no sent notification for the mail key could be found' do + let(:email_raw) { fixture_file('emails/wrong_mail_key.eml') } + + it 'raises a SentNotificationNotFoundError' do + expect { receiver.execute }.to raise_error(Gitlab::Email::SentNotificationNotFoundError) + end + end +end diff --git a/spec/lib/gitlab/incoming_email_spec.rb b/spec/lib/gitlab/incoming_email_spec.rb index 1dcf2c0668b..7e951e3fcdd 100644 --- a/spec/lib/gitlab/incoming_email_spec.rb +++ b/spec/lib/gitlab/incoming_email_spec.rb @@ -23,6 +23,48 @@ describe Gitlab::IncomingEmail, lib: true do end end + describe 'self.supports_wildcard?' do + context 'address contains the wildard placeholder' do + before do + stub_incoming_email_setting(address: 'replies+%{key}@example.com') + end + + it 'confirms that wildcard is supported' do + expect(described_class.supports_wildcard?).to be_truthy + end + end + + context "address doesn't contain the wildcard placeholder" do + before do + stub_incoming_email_setting(address: 'replies@example.com') + end + + it 'returns that wildcard is not supported' do + expect(described_class.supports_wildcard?).to be_falsey + end + end + + context 'address is not set' do + before do + stub_incoming_email_setting(address: nil) + end + + it 'returns that wildard is not supported' do + expect(described_class.supports_wildcard?).to be_falsey + end + end + end + + context 'self.unsubscribe_address' do + before do + stub_incoming_email_setting(address: 'replies+%{key}@example.com') + end + + it 'returns the address with interpolated reply key and unsubscribe suffix' do + expect(described_class.unsubscribe_address('key')).to eq('replies+key+unsubscribe@example.com') + end + end + context "self.reply_address" do before do stub_incoming_email_setting(address: "replies+%{key}@example.com") diff --git a/spec/support/notify_shared_examples.rb b/spec/support/notify_shared_examples.rb index 49867aa5cc4..a3724b801b3 100644 --- a/spec/support/notify_shared_examples.rb +++ b/spec/support/notify_shared_examples.rb @@ -179,9 +179,24 @@ shared_examples 'it should show Gmail Actions View Commit link' do end shared_examples 'an unsubscribeable thread' do + it_behaves_like 'an unsubscribeable thread with incoming address without %{key}' + + it 'has a List-Unsubscribe header in the correct format' do + is_expected.to have_header 'List-Unsubscribe', /unsubscribe/ + is_expected.to have_header 'List-Unsubscribe', /mailto/ + is_expected.to have_header 'List-Unsubscribe', /^<.+,.+>$/ + end + + it { is_expected.to have_body_text /unsubscribe/ } +end + +shared_examples 'an unsubscribeable thread with incoming address without %{key}' do + include_context 'reply-by-email is enabled with incoming address without %{key}' + it 'has a List-Unsubscribe header in the correct format' do is_expected.to have_header 'List-Unsubscribe', /unsubscribe/ - is_expected.to have_header 'List-Unsubscribe', /^<.+>$/ + is_expected.not_to have_header 'List-Unsubscribe', /mailto/ + is_expected.to have_header 'List-Unsubscribe', /^<[^,]+>$/ end it { is_expected.to have_body_text /unsubscribe/ } -- cgit v1.2.1 From 683ea89e04d115352b0411be79594475932edd6e Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 12 Jan 2017 14:19:34 -0500 Subject: Use bootstrap dropdown events to only make the request when the dropdown is being opened Fixes builds dropdown making request when clicked to be closed Adds MR ID to CHANGELOG Improve documentation Use bootstrap dropdown events to only make the request when the dropdown is being opened --- .../javascripts/mini_pipeline_graph_dropdown.js.es6 | 21 +++++++++++---------- .../26601-dropdown-makes-request-close.yml | 4 ++++ .../fixtures/mini_dropdown_graph.html.haml | 11 ++++++----- 3 files changed, 21 insertions(+), 15 deletions(-) create mode 100644 changelogs/unreleased/26601-dropdown-makes-request-close.yml diff --git a/app/assets/javascripts/mini_pipeline_graph_dropdown.js.es6 b/app/assets/javascripts/mini_pipeline_graph_dropdown.js.es6 index 90b3366f14b..80549532ea9 100644 --- a/app/assets/javascripts/mini_pipeline_graph_dropdown.js.es6 +++ b/app/assets/javascripts/mini_pipeline_graph_dropdown.js.es6 @@ -10,9 +10,9 @@ * The container should be the table element. * * The stage icon clicked needs to have the following HTML structure: - *
- * - *
+ *
+ * + *
*
*/ (() => { @@ -26,13 +26,11 @@ } /** - * Adds and removes the event listener. + * Adds the event listener when the dropdown is opened. + * All dropdown events are fired at the .dropdown-menu's parent element. */ bindEvents() { - const dropdownButtonSelector = 'button.js-builds-dropdown-button'; - - $(this.container).off('click', dropdownButtonSelector, this.getBuildsList) - .on('click', dropdownButtonSelector, this.getBuildsList); + $(this.container).on('shown.bs.dropdown', this.getBuildsList); } /** @@ -52,11 +50,14 @@ /** * For the clicked stage, gets the list of builds. * - * @param {Object} e + * All dropdown events have a relatedTarget property, + * whose value is the toggling anchor element. + * + * @param {Object} e bootstrap dropdown event * @return {Promise} */ getBuildsList(e) { - const button = e.currentTarget; + const button = e.relatedTarget; const endpoint = button.dataset.stageEndpoint; return $.ajax({ diff --git a/changelogs/unreleased/26601-dropdown-makes-request-close.yml b/changelogs/unreleased/26601-dropdown-makes-request-close.yml new file mode 100644 index 00000000000..a810e04376d --- /dev/null +++ b/changelogs/unreleased/26601-dropdown-makes-request-close.yml @@ -0,0 +1,4 @@ +--- +title: Fixes builds dropdown making request when clicked to be closed +merge_request: 8545 +author: diff --git a/spec/javascripts/fixtures/mini_dropdown_graph.html.haml b/spec/javascripts/fixtures/mini_dropdown_graph.html.haml index e9bf7568e95..29370b974af 100644 --- a/spec/javascripts/fixtures/mini_dropdown_graph.html.haml +++ b/spec/javascripts/fixtures/mini_dropdown_graph.html.haml @@ -1,8 +1,9 @@ -%div.js-builds-dropdown-tests - %button.dropdown.js-builds-dropdown-button{'data-stage-endpoint' => 'foobar'} +%div.js-builds-dropdown-tests.dropdown.dropdown.js-mini-pipeline-graph + %button.js-builds-dropdown-button{'data-stage-endpoint' => 'foobar', data: { toggle: 'dropdown'} } Dropdown - %div.js-builds-dropdown-container - %div.js-builds-dropdown-list - %div.js-builds-dropdown-loading.builds-dropdown-loading.hidden + %ul.dropdown-menu.mini-pipeline-graph-dropdown-menu.js-builds-dropdown-container + .js-builds-dropdown-list.scrollable-menu + + .js-builds-dropdown-loading.builds-dropdown-loading.hidden %span.fa.fa-spinner.fa-spin -- cgit v1.2.1 From eb9b96405498e37b25aa32876b0e101d1880f4e9 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 5 Jan 2017 12:40:54 +0100 Subject: Allow creating protected branch when it doesn't exist if user has either push or merge permissions + Change log entry for fix to creating a branch matching a wildcard fails --- .../22638-creating-a-branch-matching-a-wildcard-fails.yml | 4 ++++ lib/gitlab/user_access.rb | 4 +++- spec/lib/gitlab/user_access_spec.rb | 9 ++++++++- 3 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/22638-creating-a-branch-matching-a-wildcard-fails.yml diff --git a/changelogs/unreleased/22638-creating-a-branch-matching-a-wildcard-fails.yml b/changelogs/unreleased/22638-creating-a-branch-matching-a-wildcard-fails.yml new file mode 100644 index 00000000000..2c6883bcf7b --- /dev/null +++ b/changelogs/unreleased/22638-creating-a-branch-matching-a-wildcard-fails.yml @@ -0,0 +1,4 @@ +--- +title: Allow creating protected branches when user can merge to such branch +merge_request: 8458 +author: diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 6c7e673fb9f..6ce9b229294 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -35,7 +35,9 @@ module Gitlab return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user) access_levels = project.protected_branches.matching(ref).map(&:push_access_levels).flatten - access_levels.any? { |access_level| access_level.check_access(user) } + has_access = access_levels.any? { |access_level| access_level.check_access(user) } + + has_access || !project.repository.branch_exists?(ref) && can_merge_to_branch?(ref) else user.can?(:push_code, project) end diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index d3c3b800b94..369e55f61f1 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -66,7 +66,8 @@ describe Gitlab::UserAccess, lib: true do end describe 'push to protected branch' do - let(:branch) { create :protected_branch, project: project } + let(:branch) { create :protected_branch, project: project, name: "test" } + let(:not_existing_branch) { create :protected_branch, :developers_can_merge, project: project } it 'returns true if user is a master' do project.team << [user, :master] @@ -85,6 +86,12 @@ describe Gitlab::UserAccess, lib: true do expect(access.can_push_to_branch?(branch.name)).to be_falsey end + + it 'returns true if branch does not exist and user has permission to merge' do + project.team << [user, :developer] + + expect(access.can_push_to_branch?(not_existing_branch.name)).to be_truthy + end end describe 'push to protected branch if allowed for developers' do -- cgit v1.2.1 From e2585e642ac53802c6c5b4c2ee2ec3e6be584981 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 13 Jan 2017 16:12:02 +0000 Subject: Rename endboss -> maintainer, miniboss -> reviewer We want to describe these roles in a way that is more understandable to people not familiar with GitLab. --- doc/development/code_review.md | 12 ++++++------ doc/development/merge_request_performance_guidelines.md | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 1ef34c79971..e4a0e0b92bc 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -9,7 +9,7 @@ code is effective, understandable, and maintainable. Any developer can, and is encouraged to, perform code review on merge requests of colleagues and contributors. However, the final decision to accept a merge -request is up to one of our merge request "endbosses", denoted on the +request is up to one the project's maintainers, denoted on the [team page](https://about.gitlab.com/team). ## Everyone @@ -81,15 +81,15 @@ balance in how deep the reviewer can interfere with the code created by a reviewee. - Learning how to find the right balance takes time; that is why we have - minibosses that become merge request endbosses after some time spent on - reviewing merge requests. + reviewers that become maintainers after some time spent on reviewing merge + requests. - Finding bugs and improving code style is important, but thinking about good design is important as well. Building abstractions and good design is what makes it possible to hide complexity and makes future changes easier. - Asking the reviewee to change the design sometimes means the complete rewrite - of the contributed code. It's usually a good idea to ask another merge - request endboss before doing it, but have the courage to do it when you - believe it is important. + of the contributed code. It's usually a good idea to ask another maintainer or + reviewer before doing it, but have the courage to do it when you believe it is + important. - There is a difference in doing things right and doing things right now. Ideally, we should do the former, but in the real world we need the latter as well. A good example is a security fix which should be released as soon as diff --git a/doc/development/merge_request_performance_guidelines.md b/doc/development/merge_request_performance_guidelines.md index 0363bf8c1d5..8232a0a113c 100644 --- a/doc/development/merge_request_performance_guidelines.md +++ b/doc/development/merge_request_performance_guidelines.md @@ -3,7 +3,7 @@ To ensure a merge request does not negatively impact performance of GitLab _every_ merge request **must** adhere to the guidelines outlined in this document. There are no exceptions to this rule unless specifically discussed -with and agreed upon by merge request endbosses and performance specialists. +with and agreed upon by backend maintainers and performance specialists. To measure the impact of a merge request you can use [Sherlock](profiling.md#sherlock). It's also highly recommended that you read @@ -40,9 +40,9 @@ section below for more information. about the impact. Sometimes it's hard to assess the impact of a merge request. In this case you -should ask one of the merge request (mini) endbosses to review your changes. You -can find a list of these endbosses at . An -endboss in turn can request a performance specialist to review the changes. +should ask one of the merge request reviewers to review your changes. You can +find a list of these reviewers at . A reviewer +in turn can request a performance specialist to review the changes. ## Query Counts -- cgit v1.2.1 From 66ccf2d9f64f8e0a13e7664daa971d001dd630fb Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Fri, 13 Jan 2017 12:20:38 -0500 Subject: Document the `auto_link_ldap_user` setting --- doc/integration/omniauth.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/doc/integration/omniauth.md b/doc/integration/omniauth.md index 4c933cef9b7..98a680d0dbe 100644 --- a/doc/integration/omniauth.md +++ b/doc/integration/omniauth.md @@ -41,6 +41,9 @@ that are in common for all providers that we need to consider. - `allow_single_sign_on` allows you to specify the providers you want to allow to automatically create an account. It defaults to `false`. If `false` users must be created manually or they will not be able to sign in via OmniAuth. +- `auto_link_ldap_user` can be used if you have [LDAP / ActiveDirectory](ldap.md) + integration enabled. It defaults to false. When enabled, users automatically + created through OmniAuth will be linked to their LDAP entry as well. - `block_auto_created_users` defaults to `true`. If `true` auto created users will be blocked by default and will have to be unblocked by an administrator before they are able to sign in. @@ -52,6 +55,10 @@ SAML, Shibboleth, Crowd or Google, or set it to `false` otherwise any user on the Internet will be able to successfully sign in to your GitLab without administrative approval. +>**Note:** +`auto_link_ldap_user` requires the `uid` of the user to be the same in both LDAP +and the OmniAuth provider. + To change these settings: * **For omnibus package** @@ -72,6 +79,7 @@ To change these settings: # using an array, e.g. ["saml", "twitter"], or as true/false to allow all providers or none. # User accounts will be created automatically when authentication was successful. gitlab_rails['omniauth_allow_single_sign_on'] = ['saml', 'twitter'] + gitlab_rails['omniauth_auto_link_ldap_user'] = true gitlab_rails['omniauth_block_auto_created_users'] = true ``` @@ -99,6 +107,8 @@ To change these settings: # User accounts will be created automatically when authentication was successful. allow_single_sign_on: ["saml", "twitter"] + auto_link_ldap_user: true + # Locks down those users until they have been cleared by the admin (default: true). block_auto_created_users: true ``` -- cgit v1.2.1 From 85e0b99471b58078e1e50494ae26eb13430d3a9f Mon Sep 17 00:00:00 2001 From: twonegatives Date: Wed, 14 Dec 2016 17:37:31 +0300 Subject: Notify the user who set auto-merge when merge conflict occurs --- app/helpers/todos_helper.rb | 1 + app/models/merge_request.rb | 4 ++++ app/models/todo.rb | 8 +++++++- app/services/todo_service.rb | 15 ++++++++++++++- app/views/dashboard/todos/_todo.html.haml | 2 +- .../23524-notify-automerge-user-of-failed-build.yml | 6 +++--- spec/factories/todos.rb | 4 ++++ spec/models/merge_request_spec.rb | 10 +++++++++- spec/services/todo_service_spec.rb | 9 +++++++++ 9 files changed, 52 insertions(+), 7 deletions(-) diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index 3c039b43f5d..56c05cb54e6 100644 --- a/app/helpers/todos_helper.rb +++ b/app/helpers/todos_helper.rb @@ -14,6 +14,7 @@ module TodosHelper when Todo::BUILD_FAILED then 'The build failed for' when Todo::MARKED then 'added a todo for' when Todo::APPROVAL_REQUIRED then 'set you as an approver for' + when Todo::UNMERGEABLE then 'Could not merge' end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b73d7acefea..22a3a5fe42e 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -91,6 +91,10 @@ class MergeRequest < ActiveRecord::Base around_transition do |merge_request, transition, block| Gitlab::Timeless.timeless(merge_request, &block) end + + after_transition unchecked: :cannot_be_merged do |merge_request, transition| + TodoService.new.merge_request_became_unmergeable(merge_request) + end end validates :source_project, presence: true, unless: [:allow_broken, :importing?, :closed_without_fork?] diff --git a/app/models/todo.rb b/app/models/todo.rb index f5ade1cc293..4c99aa0d3be 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -6,13 +6,15 @@ class Todo < ActiveRecord::Base BUILD_FAILED = 3 MARKED = 4 APPROVAL_REQUIRED = 5 # This is an EE-only feature + UNMERGEABLE = 6 ACTION_NAMES = { ASSIGNED => :assigned, MENTIONED => :mentioned, BUILD_FAILED => :build_failed, MARKED => :marked, - APPROVAL_REQUIRED => :approval_required + APPROVAL_REQUIRED => :approval_required, + UNMERGEABLE => :unmergeable } belongs_to :author, class_name: "User" @@ -66,6 +68,10 @@ class Todo < ActiveRecord::Base end end + def unmergeable? + action == UNMERGEABLE + end + def build_failed? action == BUILD_FAILED end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index d67c166275a..1bd6ce416ab 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -123,7 +123,15 @@ class TodoService mark_pending_todos_as_done(merge_request, merge_request.author) mark_pending_todos_as_done(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds? end - + + # When a merge request could not be automatically merged due to its unmergeable state we should: + # + # * create a todo for a merge_user + # + def merge_request_became_unmergeable(merge_request) + create_unmergeable_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_build_succeeds? + end + # When create a note we should: # # * mark all pending todos related to the noteable for the note author as done @@ -245,6 +253,11 @@ class TodoService create_todos(todo_author, attributes) end + def create_unmergeable_todo(merge_request, merge_user) + attributes = attributes_for_todo(merge_request.project, merge_request, merge_user, Todo::UNMERGEABLE) + create_todos(merge_user, attributes) + end + def attributes_for_target(target) attributes = { project_id: target.project.id, diff --git a/app/views/dashboard/todos/_todo.html.haml b/app/views/dashboard/todos/_todo.html.haml index cc077fad32a..7fe175c1419 100644 --- a/app/views/dashboard/todos/_todo.html.haml +++ b/app/views/dashboard/todos/_todo.html.haml @@ -3,7 +3,7 @@ .todo-item.todo-block .todo-title.title - - unless todo.build_failed? + - unless todo.build_failed? || todo.unmergeable? = todo_target_state_pill(todo) %span.author-name diff --git a/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml b/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml index 4b6f9d1efe9..268be6b9b83 100644 --- a/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml +++ b/changelogs/unreleased/23524-notify-automerge-user-of-failed-build.yml @@ -1,4 +1,4 @@ --- -title: Create a TODO for user who set auto-merge when a build fails -merge_request: -author: +title: Create a TODO for user who set auto-merge when a build fails, merge conflict occurs +merge_request: 8056 +author: twonegatives diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb index 866e663f026..dc247e2d96d 100644 --- a/spec/factories/todos.rb +++ b/spec/factories/todos.rb @@ -27,6 +27,10 @@ FactoryGirl.define do action { Todo::APPROVAL_REQUIRED } end + trait :unmergeable do + action { Todo::UNMERGEABLE } + end + trait :done do state :done end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 1b71d00eb8f..59e629edf4c 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -740,10 +740,12 @@ describe MergeRequest, models: true do subject { create(:merge_request, source_project: project, merge_status: :unchecked) } context 'when it is not broken and has no conflicts' do - it 'is marked as mergeable' do + before do allow(subject).to receive(:broken?) { false } allow(project.repository).to receive(:can_be_merged?).and_return(true) + end + it 'is marked as mergeable' do expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged') end end @@ -754,6 +756,12 @@ describe MergeRequest, models: true do it 'becomes unmergeable' do expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') end + + it 'creates Todo on unmergeability' do + expect_any_instance_of(TodoService).to receive(:merge_request_became_unmergeable).with(subject) + + subject.check_if_can_be_merged + end end context 'when it has conflicts' do diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index d8a9ca20b36..13d584a8975 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -489,6 +489,15 @@ describe TodoService, services: true do end end + describe '#merge_request_became_unmergeable' do + it 'creates a pending todo for a merge_user' do + mr_unassigned.update(merge_when_build_succeeds: true, merge_user: admin) + service.merge_request_became_unmergeable(mr_unassigned) + + should_create_todo(user: admin, author: admin, target: mr_unassigned, action: Todo::UNMERGEABLE) + end + end + describe '#mark_todo' do it 'creates a todo from a merge request' do service.mark_todo(mr_unassigned, author) -- cgit v1.2.1 From b038c53073b191df2044f96d4ff5d01a35b22d83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Sat, 14 Jan 2017 00:18:21 -0500 Subject: Move default values to ApplicationSetting::DEFAULTS and use it in CurrentSettings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/application_setting.rb | 84 ++++++++++++++++++++------------------- lib/gitlab/current_settings.rb | 30 +------------- 2 files changed, 45 insertions(+), 69 deletions(-) diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 8fab77cda0a..e33a58d3771 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -13,6 +13,49 @@ class ApplicationSetting < ActiveRecord::Base [\r\n] # any number of newline characters }x + DEFAULTS_CE = { + after_sign_up_text: nil, + akismet_enabled: false, + container_registry_token_expire_delay: 5, + default_branch_protection: Settings.gitlab['default_branch_protection'], + default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'], + default_projects_limit: Settings.gitlab['default_projects_limit'], + default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'], + disabled_oauth_sign_in_sources: [], + domain_whitelist: Settings.gitlab['domain_whitelist'], + gravatar_enabled: Settings.gravatar['enabled'], + help_page_text: nil, + housekeeping_bitmaps_enabled: true, + housekeeping_enabled: true, + housekeeping_full_repack_period: 50, + housekeeping_gc_period: 200, + housekeeping_incremental_repack_period: 10, + import_sources: Gitlab::ImportSources.values, + koding_enabled: false, + koding_url: nil, + max_artifacts_size: Settings.artifacts['max_size'], + max_attachment_size: Settings.gitlab['max_attachment_size'], + plantuml_enabled: false, + plantuml_url: nil, + recaptcha_enabled: false, + repository_checks_enabled: true, + repository_storages: ['default'], + require_two_factor_authentication: false, + restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels'], + session_expire_delay: Settings.gitlab['session_expire_delay'], + send_user_confirmation_email: false, + shared_runners_enabled: Settings.gitlab_ci['shared_runners_enabled'], + shared_runners_text: nil, + sidekiq_throttling_enabled: false, + sign_in_text: nil, + signin_enabled: Settings.gitlab['signin_enabled'], + signup_enabled: Settings.gitlab['signup_enabled'], + two_factor_grace_period: 48, + user_default_external: false + } + + DEFAULTS = DEFAULTS_CE + serialize :restricted_visibility_levels serialize :import_sources serialize :disabled_oauth_sign_in_sources, Array @@ -163,46 +206,7 @@ class ApplicationSetting < ActiveRecord::Base end def self.create_from_defaults - create( - default_projects_limit: Settings.gitlab['default_projects_limit'], - default_branch_protection: Settings.gitlab['default_branch_protection'], - signup_enabled: Settings.gitlab['signup_enabled'], - signin_enabled: Settings.gitlab['signin_enabled'], - gravatar_enabled: Settings.gravatar['enabled'], - 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'], - default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'], - default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'], - domain_whitelist: Settings.gitlab['domain_whitelist'], - import_sources: Gitlab::ImportSources.values, - shared_runners_enabled: Settings.gitlab_ci['shared_runners_enabled'], - max_artifacts_size: Settings.artifacts['max_size'], - require_two_factor_authentication: false, - two_factor_grace_period: 48, - recaptcha_enabled: false, - akismet_enabled: false, - koding_enabled: false, - koding_url: nil, - plantuml_enabled: false, - plantuml_url: nil, - repository_checks_enabled: true, - disabled_oauth_sign_in_sources: [], - send_user_confirmation_email: false, - container_registry_token_expire_delay: 5, - repository_storages: ['default'], - user_default_external: false, - sidekiq_throttling_enabled: false, - housekeeping_enabled: true, - housekeeping_bitmaps_enabled: true, - housekeeping_incremental_repack_period: 10, - housekeeping_full_repack_period: 50, - housekeeping_gc_period: 200, - ) + create(DEFAULTS) end def home_page_url_column_exist diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 2ff27e46d64..c4fc3709c93 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -28,35 +28,7 @@ module Gitlab end def fake_application_settings - OpenStruct.new( - default_projects_limit: Settings.gitlab['default_projects_limit'], - default_branch_protection: Settings.gitlab['default_branch_protection'], - signup_enabled: Settings.gitlab['signup_enabled'], - signin_enabled: Settings.gitlab['signin_enabled'], - gravatar_enabled: Settings.gravatar['enabled'], - koding_enabled: false, - plantuml_enabled: false, - 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'], - default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'], - default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'], - domain_whitelist: Settings.gitlab['domain_whitelist'], - import_sources: %w[gitea github bitbucket gitlab google_code fogbugz git gitlab_project], - shared_runners_enabled: Settings.gitlab_ci['shared_runners_enabled'], - max_artifacts_size: Settings.artifacts['max_size'], - require_two_factor_authentication: false, - two_factor_grace_period: 48, - akismet_enabled: false, - repository_checks_enabled: true, - container_registry_token_expire_delay: 5, - user_default_external: false, - sidekiq_throttling_enabled: false, - ) + ApplicationSetting.new(ApplicationSetting::DEFAULTS) end private -- cgit v1.2.1 From f6cc29ed83921c3dce98d8c519c4826e7cc8221a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Sat, 14 Jan 2017 00:18:40 -0500 Subject: Don't persist ApplicationSetting in test env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- lib/gitlab/current_settings.rb | 16 ++++- lib/gitlab/github_import/project_creator.rb | 4 +- spec/controllers/health_check_controller_spec.rb | 6 ++ .../admin_disables_git_access_protocol_spec.rb | 3 + spec/features/admin/admin_health_check_spec.rb | 9 ++- spec/features/admin/admin_runners_spec.rb | 3 + spec/features/admin/admin_settings_spec.rb | 5 +- .../admin/admin_uses_repository_checks_spec.rb | 9 ++- spec/lib/gitlab/current_settings_spec.rb | 68 +++++++++++++++------- spec/requests/api/internal_spec.rb | 9 +-- spec/spec_helper.rb | 1 + 11 files changed, 97 insertions(+), 36 deletions(-) diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index c4fc3709c93..c79e17b57ee 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -9,7 +9,9 @@ module Gitlab end def ensure_application_settings! - if connect_to_db? + return fake_application_settings unless connect_to_db? + + unless ENV['IN_MEMORY_APPLICATION_SETTINGS'] == 'true' begin settings = ::ApplicationSetting.current # In case Redis isn't running or the Redis UNIX socket file is not available @@ -20,15 +22,23 @@ module Gitlab settings ||= ::ApplicationSetting.create_from_defaults unless ActiveRecord::Migrator.needs_migration? end - settings || fake_application_settings + settings || in_memory_application_settings end def sidekiq_throttling_enabled? current_application_settings.sidekiq_throttling_enabled? end + def in_memory_application_settings + @in_memory_application_settings ||= ApplicationSetting.new(ApplicationSetting::DEFAULTS) + # In case migrations the application_settings table is not created yet, + # we fallback to a simple OpenStruct + rescue ActiveRecord::StatementInvalid + fake_application_settings + end + def fake_application_settings - ApplicationSetting.new(ApplicationSetting::DEFAULTS) + OpenStruct.new(ApplicationSetting::DEFAULTS) end private diff --git a/lib/gitlab/github_import/project_creator.rb b/lib/gitlab/github_import/project_creator.rb index 3f635be22ba..a55adc9b1c8 100644 --- a/lib/gitlab/github_import/project_creator.rb +++ b/lib/gitlab/github_import/project_creator.rb @@ -1,6 +1,8 @@ module Gitlab module GithubImport class ProjectCreator + include Gitlab::CurrentSettings + attr_reader :repo, :name, :namespace, :current_user, :session_data, :type def initialize(repo, name, namespace, current_user, session_data, type: 'github') @@ -34,7 +36,7 @@ module Gitlab end def visibility_level - repo.private ? Gitlab::VisibilityLevel::PRIVATE : ApplicationSetting.current.default_project_visibility + repo.private ? Gitlab::VisibilityLevel::PRIVATE : current_application_settings.default_project_visibility end # diff --git a/spec/controllers/health_check_controller_spec.rb b/spec/controllers/health_check_controller_spec.rb index 56ecf2bb644..cfe18dd4b6c 100644 --- a/spec/controllers/health_check_controller_spec.rb +++ b/spec/controllers/health_check_controller_spec.rb @@ -1,10 +1,16 @@ require 'spec_helper' describe HealthCheckController do + include StubENV + let(:token) { current_application_settings.health_check_access_token } let(:json_response) { JSON.parse(response.body) } let(:xml_response) { Hash.from_xml(response.body)['hash'] } + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + end + describe 'GET #index' do context 'when services are up but NO access token' do it 'returns a not found page' do diff --git a/spec/features/admin/admin_disables_git_access_protocol_spec.rb b/spec/features/admin/admin_disables_git_access_protocol_spec.rb index 66044b44495..e8e080ce3e2 100644 --- a/spec/features/admin/admin_disables_git_access_protocol_spec.rb +++ b/spec/features/admin/admin_disables_git_access_protocol_spec.rb @@ -1,10 +1,13 @@ require 'rails_helper' feature 'Admin disables Git access protocol', feature: true do + include StubENV + let(:project) { create(:empty_project, :empty_repo) } let(:admin) { create(:admin) } background do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') login_as(admin) end diff --git a/spec/features/admin/admin_health_check_spec.rb b/spec/features/admin/admin_health_check_spec.rb index dec2dedf2b5..f7e49a56deb 100644 --- a/spec/features/admin/admin_health_check_spec.rb +++ b/spec/features/admin/admin_health_check_spec.rb @@ -1,9 +1,11 @@ require 'spec_helper' feature "Admin Health Check", feature: true do + include StubENV include WaitForAjax before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') login_as :admin end @@ -12,11 +14,12 @@ feature "Admin Health Check", feature: true do visit admin_health_check_path end - it { page.has_text? 'Health Check' } - it { page.has_text? 'Health information can be retrieved' } - it 'has a health check access token' do + page.has_text? 'Health Check' + page.has_text? 'Health information can be retrieved' + token = current_application_settings.health_check_access_token + expect(page).to have_content("Access token is #{token}") expect(page).to have_selector('#health-check-token', text: token) end diff --git a/spec/features/admin/admin_runners_spec.rb b/spec/features/admin/admin_runners_spec.rb index d92c66b689d..f05fbe3d062 100644 --- a/spec/features/admin/admin_runners_spec.rb +++ b/spec/features/admin/admin_runners_spec.rb @@ -1,7 +1,10 @@ require 'spec_helper' describe "Admin Runners" do + include StubENV + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') login_as :admin end diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 47fa2f14307..de42ab81fac 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -1,7 +1,10 @@ require 'spec_helper' feature 'Admin updates settings', feature: true do - before(:each) do + include StubENV + + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') login_as :admin visit admin_application_settings_path end diff --git a/spec/features/admin/admin_uses_repository_checks_spec.rb b/spec/features/admin/admin_uses_repository_checks_spec.rb index 661fb761809..855247de2ea 100644 --- a/spec/features/admin/admin_uses_repository_checks_spec.rb +++ b/spec/features/admin/admin_uses_repository_checks_spec.rb @@ -1,7 +1,12 @@ require 'rails_helper' feature 'Admin uses repository checks', feature: true do - before { login_as :admin } + include StubENV + + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + login_as :admin + end scenario 'to trigger a single check' do project = create(:empty_project) @@ -29,7 +34,7 @@ feature 'Admin uses repository checks', feature: true do scenario 'to clear all repository checks', js: true do visit admin_application_settings_path - + expect(RepositoryCheck::ClearWorker).to receive(:perform_async) click_link 'Clear all repository checks' diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb index 004341ffd02..b01c4805a34 100644 --- a/spec/lib/gitlab/current_settings_spec.rb +++ b/spec/lib/gitlab/current_settings_spec.rb @@ -1,36 +1,64 @@ require 'spec_helper' describe Gitlab::CurrentSettings do + include StubENV + + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + end + describe '#current_application_settings' do - it 'attempts to use cached values first' do - allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) - expect(ApplicationSetting).to receive(:current).and_return(::ApplicationSetting.create_from_defaults) - expect(ApplicationSetting).not_to receive(:last) + context 'with DB available' do + before do + allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) + end - expect(current_application_settings).to be_a(ApplicationSetting) - end + it 'attempts to use cached values first' do + expect(ApplicationSetting).to receive(:current) + expect(ApplicationSetting).not_to receive(:last) + + expect(current_application_settings).to be_a(ApplicationSetting) + end - it 'does not attempt to connect to DB or Redis' do - allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(false) - expect(ApplicationSetting).not_to receive(:current) - expect(ApplicationSetting).not_to receive(:last) + it 'falls back to DB if Redis returns an empty value' do + expect(ApplicationSetting).to receive(:last).and_call_original - expect(current_application_settings).to eq fake_application_settings + expect(current_application_settings).to be_a(ApplicationSetting) + end + + it 'falls back to DB if Redis fails' do + expect(ApplicationSetting).to receive(:current).and_raise(::Redis::BaseError) + expect(ApplicationSetting).to receive(:last).and_call_original + + expect(current_application_settings).to be_a(ApplicationSetting) + end end - it 'falls back to DB if Redis returns an empty value' do - allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) - expect(ApplicationSetting).to receive(:last).and_call_original + context 'with DB unavailable' do + before do + allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(false) + end - expect(current_application_settings).to be_a(ApplicationSetting) + it 'returns an in-memory ApplicationSetting object' do + expect(ApplicationSetting).not_to receive(:current) + expect(ApplicationSetting).not_to receive(:last) + + expect(current_application_settings).to be_a(OpenStruct) + end end - it 'falls back to DB if Redis fails' do - allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) - expect(ApplicationSetting).to receive(:current).and_raise(::Redis::BaseError) - expect(ApplicationSetting).to receive(:last).and_call_original + context 'when ENV["IN_MEMORY_APPLICATION_SETTINGS"] is true' do + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'true') + end + + it 'returns an in-memory ApplicationSetting object' do + expect(ApplicationSetting).not_to receive(:current) + expect(ApplicationSetting).not_to receive(:last) - expect(current_application_settings).to be_a(ApplicationSetting) + expect(current_application_settings).to be_a(ApplicationSetting) + expect(current_application_settings).not_to be_persisted + end end end end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 35644bd8cc9..c0a01d37990 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -337,8 +337,7 @@ describe API::Internal, api: true do context 'ssh access has been disabled' do before do - settings = ::ApplicationSetting.create_from_defaults - settings.update_attribute(:enabled_git_access_protocol, 'http') + stub_application_setting(enabled_git_access_protocol: 'http') end it 'rejects the SSH push' do @@ -360,8 +359,7 @@ describe API::Internal, api: true do context 'http access has been disabled' do before do - settings = ::ApplicationSetting.create_from_defaults - settings.update_attribute(:enabled_git_access_protocol, 'ssh') + stub_application_setting(enabled_git_access_protocol: 'ssh') end it 'rejects the HTTP push' do @@ -383,8 +381,7 @@ describe API::Internal, api: true do context 'web actions are always allowed' do it 'allows WEB push' do - settings = ::ApplicationSetting.create_from_defaults - settings.update_attribute(:enabled_git_access_protocol, 'ssh') + stub_application_setting(enabled_git_access_protocol: 'ssh') project.team << [user, :developer] push(key, project, 'web') diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6ee3307512d..f78899134d5 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -2,6 +2,7 @@ require './spec/simplecov_env' SimpleCovEnv.start! ENV["RAILS_ENV"] ||= 'test' +ENV["IN_MEMORY_APPLICATION_SETTINGS"] = 'true' require File.expand_path("../../config/environment", __FILE__) require 'rspec/rails' -- cgit v1.2.1 From cf3be218e17d147cb9b1395af6b968d47b56ce0a Mon Sep 17 00:00:00 2001 From: Adam Niedzielski Date: Fri, 13 Jan 2017 10:12:27 -0500 Subject: Speed up dashboard milestone index by scoping IssuesFinder to user authorized projects It improves performance in dashboard milestone index page by passing a hint to "IssuesFinder". "IssuesFinder" generates a more performant query when it is limited to authorized projects for user. In the dashboard we already limit the projects to these authorized for user (see "Dashboard::ApplicationController#projects"), so we can safely pass this option to "IssuesFinder". --- app/controllers/dashboard/milestones_controller.rb | 4 ++-- app/models/dashboard_milestone.rb | 5 +++++ changelogs/unreleased/speed-up-dashboard-milestone-index.yml | 5 +++++ 3 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 app/models/dashboard_milestone.rb create mode 100644 changelogs/unreleased/speed-up-dashboard-milestone-index.yml diff --git a/app/controllers/dashboard/milestones_controller.rb b/app/controllers/dashboard/milestones_controller.rb index 7051652d109..7f506db583f 100644 --- a/app/controllers/dashboard/milestones_controller.rb +++ b/app/controllers/dashboard/milestones_controller.rb @@ -19,11 +19,11 @@ class Dashboard::MilestonesController < Dashboard::ApplicationController private def milestones - @milestones = GlobalMilestone.build_collection(@projects, params) + @milestones = DashboardMilestone.build_collection(@projects, params) end def milestone - @milestone = GlobalMilestone.build(@projects, params[:title]) + @milestone = DashboardMilestone.build(@projects, params[:title]) render_404 unless @milestone end end diff --git a/app/models/dashboard_milestone.rb b/app/models/dashboard_milestone.rb new file mode 100644 index 00000000000..646c1e5ce1a --- /dev/null +++ b/app/models/dashboard_milestone.rb @@ -0,0 +1,5 @@ +class DashboardMilestone < GlobalMilestone + def issues_finder_params + { authorized_only: true } + end +end diff --git a/changelogs/unreleased/speed-up-dashboard-milestone-index.yml b/changelogs/unreleased/speed-up-dashboard-milestone-index.yml new file mode 100644 index 00000000000..ba4ff931ea8 --- /dev/null +++ b/changelogs/unreleased/speed-up-dashboard-milestone-index.yml @@ -0,0 +1,5 @@ +--- +title: Speed up dashboard milestone index by scoping IssuesFinder to user authorized + projects +merge_request: 8524 +author: -- cgit v1.2.1 From 725b16543d75b82fbcd858ce9d2c88fbe92cc450 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 16 Jan 2017 11:59:21 -0500 Subject: Filter environments visibility in store instead of the view in order to not get a infinite update loop in vue.js --- .../environments/components/environment.js.es6 | 45 ++--------------- .../environments/stores/environments_store.js.es6 | 59 ++++++++++++++++++++++ 2 files changed, 62 insertions(+), 42 deletions(-) diff --git a/app/assets/javascripts/environments/components/environment.js.es6 b/app/assets/javascripts/environments/components/environment.js.es6 index 6e450df3c41..fea642467fa 100644 --- a/app/assets/javascripts/environments/components/environment.js.es6 +++ b/app/assets/javascripts/environments/components/environment.js.es6 @@ -11,41 +11,6 @@ (() => { window.gl = window.gl || {}; - /** - * Given the visibility prop provided by the url query parameter and which - * changes according to the active tab we need to filter which environments - * should be visible. - * - * The environments array is a recursive tree structure and we need to filter - * both root level environments and children environments. - * - * In order to acomplish that, both `filterState` and `filterEnvironmentsByState` - * functions work together. - * The first one works as the filter that verifies if the given environment matches - * the given state. - * The second guarantees both root level and children elements are filtered as well. - */ - - const filterState = state => environment => environment.state === state && environment; - /** - * Given the filter function and the array of environments will return only - * the environments that match the state provided to the filter function. - * - * @param {Function} fn - * @param {Array} array - * @return {Array} - */ - const filterEnvironmentsByState = (fn, arr) => arr.map((item) => { - if (item.children) { - const filteredChildren = filterEnvironmentsByState(fn, item.children).filter(Boolean); - if (filteredChildren.length) { - item.children = filteredChildren; - return item; - } - } - return fn(item); - }).filter(Boolean); - gl.environmentsList.EnvironmentsComponent = Vue.component('environment-component', { props: { store: { @@ -82,10 +47,6 @@ }, computed: { - filteredEnvironments() { - return filterEnvironmentsByState(filterState(this.visibility), this.state.environments); - }, - scope() { return this.$options.getQueryParameter('scope'); }, @@ -112,7 +73,7 @@ const scope = this.$options.getQueryParameter('scope'); if (scope) { - this.visibility = scope; + this.store.storeVisibility(scope); } this.isLoading = true; @@ -213,7 +174,7 @@
+ v-if="!isLoading && state.filteredEnvironments.length > 0"> @@ -226,7 +187,7 @@ -