From 3ad0ee8bc2b1a1a5f9522e943371c6e8fda802d4 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 13 Feb 2018 19:08:47 +0000 Subject: Merge branch 'remove_ldap_person_validation' into 'master' Remove problematic LDAP::Person validation for new strategy Closes #42412 and #42359 See merge request gitlab-org/gitlab-ce!16727 --- changelogs/unreleased/remove_ldap_person_validation.yml | 5 +++++ lib/gitlab/ldap/person.rb | 15 --------------- spec/lib/gitlab/ldap/person_spec.rb | 9 --------- 3 files changed, 5 insertions(+), 24 deletions(-) create mode 100644 changelogs/unreleased/remove_ldap_person_validation.yml diff --git a/changelogs/unreleased/remove_ldap_person_validation.yml b/changelogs/unreleased/remove_ldap_person_validation.yml new file mode 100644 index 00000000000..da7f0a52886 --- /dev/null +++ b/changelogs/unreleased/remove_ldap_person_validation.yml @@ -0,0 +1,5 @@ +--- +title: LDAP Person no longer throws exception on invalid entry +merge_request: +author: +type: fixed diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index e81cec6ba1a..ec41f0ea090 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -63,8 +63,6 @@ module Gitlab Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" } @entry = entry @provider = provider - - validate_entry end def name @@ -115,19 +113,6 @@ module Gitlab entry.public_send(selected_attr) # rubocop:disable GitlabSecurity/PublicSend end - - def validate_entry - allowed_attrs = self.class.ldap_attributes(config).map(&:downcase) - - # Net::LDAP::Entry transforms keys to symbols. Change to strings to compare. - entry_attrs = entry.attribute_names.map { |n| n.to_s.downcase } - invalid_attrs = entry_attrs - allowed_attrs - - if invalid_attrs.any? - raise InvalidEntryError, - "#{self.class.name} initialized with Net::LDAP::Entry containing invalid attributes(s): #{invalid_attrs}" - end - end end end end diff --git a/spec/lib/gitlab/ldap/person_spec.rb b/spec/lib/gitlab/ldap/person_spec.rb index ff29d9aa5be..381bf2bc0e0 100644 --- a/spec/lib/gitlab/ldap/person_spec.rb +++ b/spec/lib/gitlab/ldap/person_spec.rb @@ -66,15 +66,6 @@ describe Gitlab::LDAP::Person do end end - describe '.validate_entry' do - it 'raises InvalidEntryError' do - entry['foo'] = 'bar' - - expect { described_class.new(entry, 'ldapmain') } - .to raise_error(Gitlab::LDAP::Person::InvalidEntryError) - end - end - describe '#name' do it 'uses the configured name attribute and handles values as an array' do name = 'John Doe' -- cgit v1.2.1 From 74b3192a295f907a03b5e8c70105c18d066f295b Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 30 Jan 2018 01:24:13 +0000 Subject: Merge branch 'sh-fix-jira-trailing-slash' into 'master' Fix JIRA not working when a trailing slash is included Closes #42494 See merge request gitlab-org/gitlab-ce!16748 --- app/models/project_services/jira_service.rb | 2 +- .../unreleased/sh-fix-jira-trailing-slash.yml | 5 +++++ spec/models/project_services/jira_service_spec.rb | 23 ++++++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/sh-fix-jira-trailing-slash.yml diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb index 2be35b6ea9d..23147d7f666 100644 --- a/app/models/project_services/jira_service.rb +++ b/app/models/project_services/jira_service.rb @@ -43,7 +43,7 @@ class JiraService < IssueTrackerService username: self.username, password: self.password, site: URI.join(url, '/').to_s, - context_path: url.path, + context_path: url.path.chomp('/'), auth_type: :basic, read_timeout: 120, use_cookies: true, diff --git a/changelogs/unreleased/sh-fix-jira-trailing-slash.yml b/changelogs/unreleased/sh-fix-jira-trailing-slash.yml new file mode 100644 index 00000000000..786f6cd3727 --- /dev/null +++ b/changelogs/unreleased/sh-fix-jira-trailing-slash.yml @@ -0,0 +1,5 @@ +--- +title: Fix JIRA not working when a trailing slash is included +merge_request: +author: +type: fixed diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb index c9b3c6cf602..1eaaadf56c5 100644 --- a/spec/models/project_services/jira_service_spec.rb +++ b/spec/models/project_services/jira_service_spec.rb @@ -3,6 +3,29 @@ require 'spec_helper' describe JiraService do include Gitlab::Routing + describe '#options' do + let(:service) do + described_class.new( + project: build_stubbed(:project), + active: true, + username: 'username', + password: 'test', + jira_issue_transition_id: 24, + url: 'http://jira.test.com/path/' + ) + end + + it 'sets the URL properly' do + # jira-ruby gem parses the URI and handles trailing slashes + # fine: https://github.com/sumoheavy/jira-ruby/blob/v1.4.1/lib/jira/http_client.rb#L59 + expect(service.options[:site]).to eq('http://jira.test.com/') + end + + it 'leaves out trailing slashes in context' do + expect(service.options[:context_path]).to eq('/path') + end + end + describe "Associations" do it { is_expected.to belong_to :project } it { is_expected.to have_one :service_hook } -- cgit v1.2.1 From 721a26d91a9064d76824141753e0050f25a7c508 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 30 Jan 2018 18:31:09 +0000 Subject: Merge branch '42160-error-500-loading-merge-request-undefined-method-index-for-nil-nilclass' into 'master' Resolve "Error 500 loading merge request: undefined method `index' for nil:NilClass" Closes #42160 See merge request gitlab-org/gitlab-ce!16795 --- app/models/concerns/discussion_on_diff.rb | 2 ++ ...g-merge-request-undefined-method-index-for-nil-nilclass.yml | 5 +++++ spec/models/concerns/discussion_on_diff_spec.rb | 10 ++++++++++ 3 files changed, 17 insertions(+) create mode 100644 changelogs/unreleased/42160-error-500-loading-merge-request-undefined-method-index-for-nil-nilclass.yml diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb index db9770fabf4..8b3c55387b3 100644 --- a/app/models/concerns/discussion_on_diff.rb +++ b/app/models/concerns/discussion_on_diff.rb @@ -37,6 +37,8 @@ module DiscussionOnDiff # Returns an array of at most 16 highlighted lines above a diff note def truncated_diff_lines(highlight: true) + return [] if diff_line.nil? && first_note.is_a?(LegacyDiffNote) + lines = highlight ? highlighted_diff_lines : diff_lines initial_line_index = [diff_line.index - NUMBER_OF_TRUNCATED_DIFF_LINES + 1, 0].max diff --git a/changelogs/unreleased/42160-error-500-loading-merge-request-undefined-method-index-for-nil-nilclass.yml b/changelogs/unreleased/42160-error-500-loading-merge-request-undefined-method-index-for-nil-nilclass.yml new file mode 100644 index 00000000000..64340ab08cd --- /dev/null +++ b/changelogs/unreleased/42160-error-500-loading-merge-request-undefined-method-index-for-nil-nilclass.yml @@ -0,0 +1,5 @@ +--- +title: Fix 500 error when loading a merge request with an invalid comment +merge_request: 16795 +author: +type: fixed diff --git a/spec/models/concerns/discussion_on_diff_spec.rb b/spec/models/concerns/discussion_on_diff_spec.rb index 2322eb206fb..30572ce9332 100644 --- a/spec/models/concerns/discussion_on_diff_spec.rb +++ b/spec/models/concerns/discussion_on_diff_spec.rb @@ -20,6 +20,16 @@ describe DiscussionOnDiff do expect(truncated_lines).not_to include(be_meta) end end + + context "when the diff line does not exist on a legacy diff note" do + it "returns an empty array" do + legacy_note = LegacyDiffNote.new + + allow(subject).to receive(:first_note).and_return(legacy_note) + + expect(truncated_lines).to eq([]) + end + end end describe '#line_code_in_diffs' do -- cgit v1.2.1 From a5c8f3fe8b18521dfa3a6c606f993435da192ebd Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 13 Feb 2018 09:45:36 +0000 Subject: Merge branch 'winh-new-branch-dropdown-style' into 'master' Cleanup new branch/merge request form in issues Closes #41938 See merge request gitlab-org/gitlab-ce!16854 --- .../javascripts/create_merge_request_dropdown.js | 1 + app/assets/javascripts/droplab/constants.js | 2 - app/assets/javascripts/droplab/drop_down.js | 13 ++- app/assets/stylesheets/framework/common.scss | 2 + app/assets/stylesheets/framework/dropdowns.scss | 4 - app/assets/stylesheets/framework/forms.scss | 1 + app/assets/stylesheets/pages/issues.scss | 76 ++------------ app/views/projects/issues/_new_branch.html.haml | 57 ++++++----- .../unreleased/winh-new-branch-dropdown-style.yml | 5 + spec/javascripts/droplab/drop_down_spec.js | 113 ++++++++++++++------- 10 files changed, 135 insertions(+), 139 deletions(-) create mode 100644 changelogs/unreleased/winh-new-branch-dropdown-style.yml diff --git a/app/assets/javascripts/create_merge_request_dropdown.js b/app/assets/javascripts/create_merge_request_dropdown.js index eedbd3feeb5..981141dc178 100644 --- a/app/assets/javascripts/create_merge_request_dropdown.js +++ b/app/assets/javascripts/create_merge_request_dropdown.js @@ -186,6 +186,7 @@ export default class CreateMergeRequestDropdown { valueAttribute: 'data-text', }, ], + hideOnClick: false, }; } diff --git a/app/assets/javascripts/droplab/constants.js b/app/assets/javascripts/droplab/constants.js index 673e9bb4c0f..868d47e91b3 100644 --- a/app/assets/javascripts/droplab/constants.js +++ b/app/assets/javascripts/droplab/constants.js @@ -3,7 +3,6 @@ const DATA_DROPDOWN = 'data-dropdown'; const SELECTED_CLASS = 'droplab-item-selected'; const ACTIVE_CLASS = 'droplab-item-active'; const IGNORE_CLASS = 'droplab-item-ignore'; -const IGNORE_HIDING_CLASS = 'droplab-item-ignore-hiding'; // Matches `{{anything}}` and `{{ everything }}`. const TEMPLATE_REGEX = /\{\{(.+?)\}\}/g; @@ -14,5 +13,4 @@ export { ACTIVE_CLASS, TEMPLATE_REGEX, IGNORE_CLASS, - IGNORE_HIDING_CLASS, }; diff --git a/app/assets/javascripts/droplab/drop_down.js b/app/assets/javascripts/droplab/drop_down.js index 5eb0a339a1c..3cc316c3f3e 100644 --- a/app/assets/javascripts/droplab/drop_down.js +++ b/app/assets/javascripts/droplab/drop_down.js @@ -1,13 +1,14 @@ import utils from './utils'; -import { SELECTED_CLASS, IGNORE_CLASS, IGNORE_HIDING_CLASS } from './constants'; +import { SELECTED_CLASS, IGNORE_CLASS } from './constants'; class DropDown { - constructor(list, config = {}) { + constructor(list, config = { }) { this.currentIndex = 0; this.hidden = true; this.list = typeof list === 'string' ? document.querySelector(list) : list; this.items = []; this.eventWrapper = {}; + this.hideOnClick = config.hideOnClick !== false; if (config.addActiveClassToDropdownButton) { this.dropdownToggle = this.list.parentNode.querySelector('.js-dropdown-toggle'); @@ -37,15 +38,17 @@ class DropDown { clickEvent(e) { if (e.target.tagName === 'UL') return; - if (e.target.classList.contains(IGNORE_CLASS)) return; + if (e.target.closest(`.${IGNORE_CLASS}`)) return; - const selected = utils.closest(e.target, 'LI'); + const selected = e.target.closest('li'); if (!selected) return; this.addSelectedClass(selected); e.preventDefault(); - if (!e.target.classList.contains(IGNORE_HIDING_CLASS)) this.hide(); + if (this.hideOnClick) { + this.hide(); + } const listEvent = new CustomEvent('click.dl', { detail: { diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 73524d5cf60..ae517c41cb2 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -449,9 +449,11 @@ img.emoji { .prepend-top-10 { margin-top: 10px; } .prepend-top-15 { margin-top: 15px; } .prepend-top-default { margin-top: $gl-padding !important; } +.prepend-top-16 { margin-top: 16px; } .prepend-top-20 { margin-top: 20px; } .prepend-left-4 { margin-left: 4px; } .prepend-left-5 { margin-left: 5px; } +.prepend-left-8 { margin-left: 8px; } .prepend-left-10 { margin-left: 10px; } .prepend-left-default { margin-left: $gl-padding; } .prepend-left-20 { margin-left: 20px; } diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index bc907a390d8..12674019583 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -729,10 +729,6 @@ } } -.droplab-item-ignore { - pointer-events: none; -} - .pika-single.animate-picker.is-bound, .pika-single.animate-picker.is-bound.is-hidden { /* diff --git a/app/assets/stylesheets/framework/forms.scss b/app/assets/stylesheets/framework/forms.scss index be96c8ee964..a2ea155a10e 100644 --- a/app/assets/stylesheets/framework/forms.scss +++ b/app/assets/stylesheets/framework/forms.scss @@ -182,6 +182,7 @@ label { .help-block { margin-bottom: 0; + margin-top: #{$grid-size / 2}; } .gl-field-error { diff --git a/app/assets/stylesheets/pages/issues.scss b/app/assets/stylesheets/pages/issues.scss index c48e58af691..6763af4e98b 100644 --- a/app/assets/stylesheets/pages/issues.scss +++ b/app/assets/stylesheets/pages/issues.scss @@ -181,11 +181,6 @@ ul.related-merge-requests > li { } .create-mr-dropdown-wrap { - .branch-message, - .ref-message { - display: none; - } - .ref::selection { color: $placeholder-text-color; } @@ -216,6 +211,17 @@ ul.related-merge-requests > li { transform: translateY(0); display: none; margin-top: 4px; + + // override dropdown item styles + .btn.btn-success { + @include btn-default; + @include btn-green; + + border-style: solid; + border-width: 1px; + line-height: $line-height-base; + width: auto; + } } .create-merge-request-dropdown-toggle { @@ -225,66 +231,6 @@ ul.related-merge-requests > li { margin-left: 0; } } - - .droplab-item-ignore { - pointer-events: auto; - } - - .create-item { - cursor: pointer; - margin: 0 1px; - - &:hover, - &:focus { - background-color: $dropdown-item-hover-bg; - color: $gl-text-color; - } - } - - li.divider { - margin: 8px 10px; - } - - li:not(.divider) { - padding: 8px 9px; - - &:last-child { - padding-bottom: 8px; - } - - &.droplab-item-selected { - .icon-container { - i { - visibility: visible; - } - } - - .description { - display: block; - } - } - - &.droplab-item-ignore { - padding-top: 8px; - } - - .icon-container { - float: left; - - i { - visibility: hidden; - } - } - - .description { - padding-left: 22px; - } - - input, - span { - margin: 4px 0 0; - } - } } .discussion-reply-holder .note-edit-form { diff --git a/app/views/projects/issues/_new_branch.html.haml b/app/views/projects/issues/_new_branch.html.haml index 331d62cf247..672bf0ce805 100644 --- a/app/views/projects/issues/_new_branch.html.haml +++ b/app/views/projects/issues/_new_branch.html.haml @@ -21,30 +21,33 @@ %button.btn.create-merge-request-dropdown-toggle.dropdown-toggle.btn-default.js-dropdown-toggle{ type: 'button', data: { dropdown: { trigger: '#create-merge-request-dropdown' } } } = icon('caret-down') - %ul#create-merge-request-dropdown.create-merge-request-dropdown-menu.dropdown-menu.dropdown-menu-align-right.gl-show-field-errors{ data: { dropdown: true } } - - if can_create_merge_request - %li.create-item.droplab-item-selected.droplab-item-ignore-hiding{ role: 'button', data: { value: 'create-mr', text: 'Create merge request' } } - .menu-item.droplab-item-ignore-hiding - .icon-container.droplab-item-ignore-hiding= icon('check') - .description.droplab-item-ignore-hiding Create merge request and branch - - %li.create-item.droplab-item-ignore-hiding{ class: [!can_create_merge_request && 'droplab-item-selected'], role: 'button', data: { value: 'create-branch', text: 'Create branch' } } - .menu-item.droplab-item-ignore-hiding - .icon-container.droplab-item-ignore-hiding= icon('check') - .description.droplab-item-ignore-hiding Create branch - %li.divider - - %li.droplab-item-ignore - Branch name - %input.js-branch-name.form-control.droplab-item-ignore{ type: 'text', placeholder: "#{@issue.to_branch_name}", value: "#{@issue.to_branch_name}" } - %span.js-branch-message.branch-message.droplab-item-ignore - - %li.droplab-item-ignore - Source (branch or tag) - %input.js-ref.ref.form-control.droplab-item-ignore{ type: 'text', placeholder: "#{@project.default_branch}", value: "#{@project.default_branch}", data: { value: "#{@project.default_branch}" } } - %span.js-ref-message.ref-message.droplab-item-ignore - - %li.droplab-item-ignore - %button.btn.btn-success.js-create-target.droplab-item-ignore{ type: 'button', data: { action: 'create-mr' } } - Create merge request - + .droplab-dropdown + %ul#create-merge-request-dropdown.create-merge-request-dropdown-menu.dropdown-menu.dropdown-menu-align-right.gl-show-field-errors{ data: { dropdown: true } } + - if can_create_merge_request + %li.droplab-item-selected{ role: 'button', data: { value: 'create-mr', text: _('Create merge request') } } + .menu-item + = icon('check', class: 'icon') + = _('Create merge request and branch') + + %li{ class: [!can_create_merge_request && 'droplab-item-selected'], role: 'button', data: { value: 'create-branch', text: _('Create branch') } } + .menu-item + = icon('check', class: 'icon') + = _('Create branch') + %li.divider.droplab-item-ignore + + %li.droplab-item-ignore.prepend-left-8.append-right-8.prepend-top-16 + .form-group + %label{ for: 'new-branch-name' } + = _('Branch name') + %input#new-branch-name.js-branch-name.form-control{ type: 'text', placeholder: "#{@issue.to_branch_name}", value: "#{@issue.to_branch_name}" } + %span.js-branch-message.help-block + + .form-group + %label{ for: 'source-name' } + = _('Source (branch or tag)') + %input#source-name.js-ref.ref.form-control{ type: 'text', placeholder: "#{@project.default_branch}", value: "#{@project.default_branch}", data: { value: "#{@project.default_branch}" } } + %span.js-ref-message.help-block + + .form-group + %button.btn.btn-success.js-create-target{ type: 'button', data: { action: 'create-mr' } } + = _('Create merge request') diff --git a/changelogs/unreleased/winh-new-branch-dropdown-style.yml b/changelogs/unreleased/winh-new-branch-dropdown-style.yml new file mode 100644 index 00000000000..007e9e9f453 --- /dev/null +++ b/changelogs/unreleased/winh-new-branch-dropdown-style.yml @@ -0,0 +1,5 @@ +--- +title: Cleanup new branch/merge request form in issues +merge_request: 16854 +author: +type: fixed diff --git a/spec/javascripts/droplab/drop_down_spec.js b/spec/javascripts/droplab/drop_down_spec.js index 1225fe2cb66..896a04a1a07 100644 --- a/spec/javascripts/droplab/drop_down_spec.js +++ b/spec/javascripts/droplab/drop_down_spec.js @@ -1,8 +1,8 @@ import DropDown from '~/droplab/drop_down'; import utils from '~/droplab/utils'; -import { SELECTED_CLASS, IGNORE_CLASS } from '~/droplab/constants'; +import { SELECTED_CLASS } from '~/droplab/constants'; -describe('DropDown', function () { +describe('DropLab DropDown', function () { describe('class constructor', function () { beforeEach(function () { spyOn(DropDown.prototype, 'getItems'); @@ -128,93 +128,131 @@ describe('DropDown', function () { beforeEach(function () { this.classList = jasmine.createSpyObj('classList', ['contains']); this.list = { dispatchEvent: () => {} }; - this.dropdown = { hide: () => {}, list: this.list, addSelectedClass: () => {} }; - this.event = { preventDefault: () => {}, target: { classList: this.classList } }; + this.dropdown = { + hideOnClick: true, + hide: () => {}, + list: this.list, + addSelectedClass: () => {}, + }; + this.event = { + preventDefault: () => {}, + target: { + classList: this.classList, + closest: () => null, + }, + }; this.customEvent = {}; - this.closestElement = {}; + this.dummyListItem = document.createElement('li'); + spyOn(this.event.target, 'closest').and.callFake((selector) => { + if (selector === 'li') { + return this.dummyListItem; + } + + return null; + }); spyOn(this.dropdown, 'hide'); spyOn(this.dropdown, 'addSelectedClass'); spyOn(this.list, 'dispatchEvent'); spyOn(this.event, 'preventDefault'); spyOn(window, 'CustomEvent').and.returnValue(this.customEvent); - spyOn(utils, 'closest').and.returnValues(this.closestElement, undefined); this.classList.contains.and.returnValue(false); + }); + it('should call event.target.closest', function () { DropDown.prototype.clickEvent.call(this.dropdown, this.event); - }); - it('should call utils.closest', function () { - expect(utils.closest).toHaveBeenCalledWith(this.event.target, 'LI'); + expect(this.event.target.closest).toHaveBeenCalledWith('.droplab-item-ignore'); + expect(this.event.target.closest).toHaveBeenCalledWith('li'); }); it('should call addSelectedClass', function () { - expect(this.dropdown.addSelectedClass).toHaveBeenCalledWith(this.closestElement); + DropDown.prototype.clickEvent.call(this.dropdown, this.event); + + expect(this.dropdown.addSelectedClass).toHaveBeenCalledWith(this.dummyListItem); }); it('should call .preventDefault', function () { + DropDown.prototype.clickEvent.call(this.dropdown, this.event); + expect(this.event.preventDefault).toHaveBeenCalled(); }); it('should call .hide', function () { + DropDown.prototype.clickEvent.call(this.dropdown, this.event); + expect(this.dropdown.hide).toHaveBeenCalled(); }); it('should construct CustomEvent', function () { - expect(window.CustomEvent).toHaveBeenCalledWith('click.dl', jasmine.any(Object)); - }); + DropDown.prototype.clickEvent.call(this.dropdown, this.event); - it('should call .classList.contains checking for IGNORE_CLASS', function () { - expect(this.classList.contains).toHaveBeenCalledWith(IGNORE_CLASS); + expect(window.CustomEvent).toHaveBeenCalledWith('click.dl', jasmine.any(Object)); }); it('should call .dispatchEvent with the customEvent', function () { + DropDown.prototype.clickEvent.call(this.dropdown, this.event); + expect(this.list.dispatchEvent).toHaveBeenCalledWith(this.customEvent); }); describe('if the target is a UL element', function () { beforeEach(function () { - this.event = { preventDefault: () => {}, target: { tagName: 'UL', classList: this.classList } }; - - spyOn(this.event, 'preventDefault'); - utils.closest.calls.reset(); + this.event.target = document.createElement('ul'); - DropDown.prototype.clickEvent.call(this.dropdown, this.event); + spyOn(this.event.target, 'closest'); }); it('should return immediately', function () { - expect(utils.closest).not.toHaveBeenCalled(); + DropDown.prototype.clickEvent.call(this.dropdown, this.event); + + expect(this.event.target.closest).not.toHaveBeenCalled(); + expect(this.dropdown.addSelectedClass).not.toHaveBeenCalled(); }); }); - describe('if the target has the IGNORE_CLASS class', function () { + describe('if the target has the droplab-item-ignore class', function () { beforeEach(function () { - this.event = { preventDefault: () => {}, target: { tagName: 'LI', classList: this.classList } }; + this.ignoredButton = document.createElement('button'); + this.ignoredButton.classList.add('droplab-item-ignore'); + this.event.target = this.ignoredButton; - spyOn(this.event, 'preventDefault'); - this.classList.contains.and.returnValue(true); - utils.closest.calls.reset(); + spyOn(this.ignoredButton, 'closest').and.callThrough(); + }); + it('does not select element', function () { DropDown.prototype.clickEvent.call(this.dropdown, this.event); - }); - it('should return immediately', function () { - expect(utils.closest).not.toHaveBeenCalled(); + expect(this.ignoredButton.closest.calls.count()).toBe(1); + expect(this.ignoredButton.closest).toHaveBeenCalledWith('.droplab-item-ignore'); + expect(this.dropdown.addSelectedClass).not.toHaveBeenCalled(); }); }); describe('if no selected element exists', function () { beforeEach(function () { this.event.preventDefault.calls.reset(); - this.clickEvent = DropDown.prototype.clickEvent.call(this.dropdown, this.event); - }); - - it('should return undefined', function () { - expect(this.clickEvent).toBe(undefined); + this.dummyListItem = null; }); it('should return before .preventDefault is called', function () { + DropDown.prototype.clickEvent.call(this.dropdown, this.event); + expect(this.event.preventDefault).not.toHaveBeenCalled(); + expect(this.dropdown.addSelectedClass).not.toHaveBeenCalled(); + }); + }); + + describe('if hideOnClick is false', () => { + beforeEach(function () { + this.dropdown.hideOnClick = false; + this.dropdown.hide.calls.reset(); + }); + + it('should not call .hide', function () { + DropDown.prototype.clickEvent.call(this.dropdown, this.event); + + expect(this.dropdown.hide).not.toHaveBeenCalled(); }); }); }); @@ -278,20 +316,23 @@ describe('DropDown', function () { describe('addEvents', function () { beforeEach(function () { - this.list = { addEventListener: () => {} }; + this.list = { + addEventListener: () => {}, + querySelectorAll: () => [], + }; this.dropdown = { list: this.list, clickEvent: () => {}, closeDropdown: () => {}, eventWrapper: {}, }; + }); + it('should call .addEventListener', function () { spyOn(this.list, 'addEventListener'); DropDown.prototype.addEvents.call(this.dropdown); - }); - it('should call .addEventListener', function () { expect(this.list.addEventListener).toHaveBeenCalledWith('click', jasmine.any(Function)); expect(this.list.addEventListener).toHaveBeenCalledWith('keyup', jasmine.any(Function)); }); -- cgit v1.2.1 From 0898e59cd07797acdfdb6924468a198b6971cfda Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 2 Feb 2018 15:14:53 +0000 Subject: Merge branch '42696-gitlab-import-leaves-group_id-on-projectlabel' into 'master' Resolve "GitLab import leaves `group_id` on ProjectLabel" Closes #42696 See merge request gitlab-org/gitlab-ce!16877 --- ...itlab-import-leaves-group_id-on-projectlabel.yml | 5 +++++ ...20180202111106_remove_project_labels_group_id.rb | 19 +++++++++++++++++++ db/schema.rb | 2 +- lib/gitlab/import_export/relation_factory.rb | 5 ++--- .../import_export/project_tree_restorer_spec.rb | 2 ++ .../remove_project_labels_group_id_spec.rb | 21 +++++++++++++++++++++ 6 files changed, 50 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/42696-gitlab-import-leaves-group_id-on-projectlabel.yml create mode 100644 db/post_migrate/20180202111106_remove_project_labels_group_id.rb create mode 100644 spec/migrations/remove_project_labels_group_id_spec.rb diff --git a/changelogs/unreleased/42696-gitlab-import-leaves-group_id-on-projectlabel.yml b/changelogs/unreleased/42696-gitlab-import-leaves-group_id-on-projectlabel.yml new file mode 100644 index 00000000000..c46cc86c99b --- /dev/null +++ b/changelogs/unreleased/42696-gitlab-import-leaves-group_id-on-projectlabel.yml @@ -0,0 +1,5 @@ +--- +title: Fix GitLab import leaving group_id on ProjectLabel +merge_request: 16877 +author: +type: fixed diff --git a/db/post_migrate/20180202111106_remove_project_labels_group_id.rb b/db/post_migrate/20180202111106_remove_project_labels_group_id.rb new file mode 100644 index 00000000000..db7fd0d167d --- /dev/null +++ b/db/post_migrate/20180202111106_remove_project_labels_group_id.rb @@ -0,0 +1,19 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RemoveProjectLabelsGroupId < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + update_column_in_batches(:labels, :group_id, nil) do |table, query| + query.where(table[:type].eq('ProjectLabel').and(table[:group_id].not_eq(nil))) + end + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index b8a29d8c046..f0d94974f41 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180113220114) do +ActiveRecord::Schema.define(version: 20180202111106) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb index 05dbaf6322c..f3562e7ca1e 100644 --- a/lib/gitlab/import_export/relation_factory.rb +++ b/lib/gitlab/import_export/relation_factory.rb @@ -139,13 +139,12 @@ module Gitlab end def setup_label - return unless @relation_hash['type'] == 'GroupLabel' - # If there's no group, move the label to a project label - if @relation_hash['group_id'] + if @relation_hash['type'] == 'GroupLabel' && @relation_hash['group_id'] @relation_hash['project_id'] = nil @relation_name = :group_label else + @relation_hash['group_id'] = nil @relation_hash['type'] = 'ProjectLabel' end end diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index 9dfd879a1bc..d076007e4bc 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -236,12 +236,14 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do labels = project.issues.first.labels expect(labels.where(type: "ProjectLabel").count).to eq(results.fetch(:first_issue_labels, 0)) + expect(labels.where(type: "ProjectLabel").where.not(group_id: nil).count).to eq(0) end end shared_examples 'restores group correctly' do |**results| it 'has group label' do expect(project.group.labels.size).to eq(results.fetch(:labels, 0)) + expect(project.group.labels.where(type: "GroupLabel").where.not(project_id: nil).count).to eq(0) end it 'has group milestone' do diff --git a/spec/migrations/remove_project_labels_group_id_spec.rb b/spec/migrations/remove_project_labels_group_id_spec.rb new file mode 100644 index 00000000000..d80d61af20b --- /dev/null +++ b/spec/migrations/remove_project_labels_group_id_spec.rb @@ -0,0 +1,21 @@ +# encoding: utf-8 + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20180202111106_remove_project_labels_group_id.rb') + +describe RemoveProjectLabelsGroupId, :delete do + let(:migration) { described_class.new } + let(:group) { create(:group) } + let!(:project_label) { create(:label, group_id: group.id) } + let!(:group_label) { create(:group_label) } + + describe '#up' do + it 'updates the project labels group ID' do + expect { migration.up }.to change { project_label.reload.group_id }.to(nil) + end + + it 'keeps the group labels group ID' do + expect { migration.up }.not_to change { group_label.reload.group_id } + end + end +end -- cgit v1.2.1 From ede2f4e5c5b62d4e96228097a19a3ef21212be42 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 2 Feb 2018 19:12:55 +0000 Subject: Merge branch 'bvl-fix-500-on-fork-without-restricted-visibility-levels' into 'master' Avoid error when creating forks and restricted levels are defined Closes #42607 See merge request gitlab-org/gitlab-ce!16881 --- .../bvl-fix-500-on-fork-without-restricted-visibility-levels.yml | 5 +++++ lib/gitlab/visibility_level.rb | 2 +- spec/lib/gitlab/visibility_level_spec.rb | 9 +++++++++ 3 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/bvl-fix-500-on-fork-without-restricted-visibility-levels.yml diff --git a/changelogs/unreleased/bvl-fix-500-on-fork-without-restricted-visibility-levels.yml b/changelogs/unreleased/bvl-fix-500-on-fork-without-restricted-visibility-levels.yml new file mode 100644 index 00000000000..378f0ef7ce9 --- /dev/null +++ b/changelogs/unreleased/bvl-fix-500-on-fork-without-restricted-visibility-levels.yml @@ -0,0 +1,5 @@ +--- +title: Fix forking projects when no restricted visibility levels are defined applicationwide +merge_request: 16881 +author: +type: fixed diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb index 6ced06a863d..0b757b2a646 100644 --- a/lib/gitlab/visibility_level.rb +++ b/lib/gitlab/visibility_level.rb @@ -60,7 +60,7 @@ module Gitlab def allowed_levels restricted_levels = current_application_settings.restricted_visibility_levels - self.values - restricted_levels + self.values - Array(restricted_levels) end def closest_allowed_level(target_level) diff --git a/spec/lib/gitlab/visibility_level_spec.rb b/spec/lib/gitlab/visibility_level_spec.rb index d85dac630b4..2c1146ceff5 100644 --- a/spec/lib/gitlab/visibility_level_spec.rb +++ b/spec/lib/gitlab/visibility_level_spec.rb @@ -57,6 +57,15 @@ describe Gitlab::VisibilityLevel do expect(described_class.allowed_levels) .to contain_exactly(described_class::PRIVATE, described_class::PUBLIC) end + + it 'returns all levels when no visibility level was set' do + allow(described_class) + .to receive_message_chain('current_application_settings.restricted_visibility_levels') + .and_return(nil) + + expect(described_class.allowed_levels) + .to contain_exactly(described_class::PRIVATE, described_class::INTERNAL, described_class::PUBLIC) + end end describe '.closest_allowed_level' do -- cgit v1.2.1 From 2008eb9900e2231f2e08f39720ff8c72189e8435 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 6 Feb 2018 18:40:09 +0000 Subject: Merge branch 'dm-user-namespace-route-path-validation' into 'master' Validate user namespace before saving so that errors persist on model Closes #42140 See merge request gitlab-org/gitlab-ce!16902 --- app/models/namespace.rb | 3 +++ app/models/user.rb | 18 ++++++------------ .../dm-user-namespace-route-path-validation.yml | 5 +++++ spec/lib/gitlab/closing_issue_extractor_spec.rb | 3 ++- spec/models/user_spec.rb | 21 +++++++++++++++++++-- .../projects/gitlab_projects_import_service_spec.rb | 2 +- 6 files changed, 36 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/dm-user-namespace-route-path-validation.yml diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 37a7417cafc..ad1597196b5 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -21,6 +21,9 @@ class Namespace < ActiveRecord::Base has_many :projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :project_statistics + + # This should _not_ be `inverse_of: :namespace`, because that would also set + # `user.namespace` when this user creates a group with themselves as `owner`. belongs_to :owner, class_name: "User" belongs_to :parent, class_name: "Namespace" diff --git a/app/models/user.rb b/app/models/user.rb index 4484ee9ff4c..d811e162102 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -76,7 +76,7 @@ class User < ActiveRecord::Base # # Namespace for personal projects - has_one :namespace, -> { where(type: nil) }, dependent: :destroy, foreign_key: :owner_id, autosave: true # rubocop:disable Cop/ActiveRecordDependent + has_one :namespace, -> { where(type: nil) }, dependent: :destroy, foreign_key: :owner_id, inverse_of: :owner, autosave: true # rubocop:disable Cop/ActiveRecordDependent # Profile has_many :keys, -> do @@ -170,7 +170,7 @@ class User < ActiveRecord::Base before_save :ensure_user_rights_and_limits, if: ->(user) { user.new_record? || user.external_changed? } before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) } before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record? } - after_save :ensure_namespace_correct + before_validation :ensure_namespace_correct after_update :username_changed_hook, if: :username_changed? after_destroy :post_destroy_hook after_destroy :remove_key_cache @@ -891,16 +891,10 @@ class User < ActiveRecord::Base end def ensure_namespace_correct - # Ensure user has namespace - create_namespace!(path: username, name: username) unless namespace - - if username_changed? - unless namespace.update_attributes(path: username, name: username) - namespace.errors.each do |attribute, message| - self.errors.add(:"namespace_#{attribute}", message) - end - raise ActiveRecord::RecordInvalid.new(namespace) - end + if namespace + namespace.path = namespace.name = username if username_changed? + else + build_namespace(path: username, name: username) end end diff --git a/changelogs/unreleased/dm-user-namespace-route-path-validation.yml b/changelogs/unreleased/dm-user-namespace-route-path-validation.yml new file mode 100644 index 00000000000..36615e5b976 --- /dev/null +++ b/changelogs/unreleased/dm-user-namespace-route-path-validation.yml @@ -0,0 +1,5 @@ +--- +title: Validate user namespace before saving so that errors persist on model +merge_request: +author: +type: fixed diff --git a/spec/lib/gitlab/closing_issue_extractor_spec.rb b/spec/lib/gitlab/closing_issue_extractor_spec.rb index 8c79ef54c6c..28c679af12a 100644 --- a/spec/lib/gitlab/closing_issue_extractor_spec.rb +++ b/spec/lib/gitlab/closing_issue_extractor_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Gitlab::ClosingIssueExtractor do let(:project) { create(:project) } let(:project2) { create(:project) } - let(:forked_project) { Projects::ForkService.new(project, project.creator).execute } + let(:forked_project) { Projects::ForkService.new(project, project2.creator).execute } let(:issue) { create(:issue, project: project) } let(:issue2) { create(:issue, project: project2) } let(:reference) { issue.to_reference } @@ -14,6 +14,7 @@ describe Gitlab::ClosingIssueExtractor do before do project.add_developer(project.creator) + project.add_developer(project2.creator) project2.add_master(project.creator) end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 8d0eaf565a7..4bd5da1025d 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -103,7 +103,7 @@ describe User do user = build(:user, username: 'dashboard') expect(user).not_to be_valid - expect(user.errors.values).to eq [['dashboard is a reserved name']] + expect(user.errors.messages[:username]).to eq ['dashboard is a reserved name'] end it 'allows child names' do @@ -134,6 +134,23 @@ describe User do expect(user.errors.messages[:username].first).to match('cannot be changed if a personal project has container registry tags') end end + + context 'when the username was used by another user before' do + let(:username) { 'foo' } + let!(:other_user) { create(:user, username: username) } + + before do + other_user.username = 'bar' + other_user.save! + end + + it 'is invalid' do + user = build(:user, username: username) + + expect(user).not_to be_valid + expect(user.errors.messages[:"namespace.route.path"].first).to eq('foo has been taken before. Please use another one') + end + end end it 'has a DB-level NOT NULL constraint on projects_limit' do @@ -2603,7 +2620,7 @@ describe User do it 'should raise an ActiveRecord::RecordInvalid exception' do user2 = build(:user, username: 'foo') - expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Path foo has been taken before/) + expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Namespace route path foo has been taken before/) end end diff --git a/spec/services/projects/gitlab_projects_import_service_spec.rb b/spec/services/projects/gitlab_projects_import_service_spec.rb index bb0e274c93e..6b8f9619bc4 100644 --- a/spec/services/projects/gitlab_projects_import_service_spec.rb +++ b/spec/services/projects/gitlab_projects_import_service_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Projects::GitlabProjectsImportService do - set(:namespace) { build(:namespace) } + set(:namespace) { create(:namespace) } let(:file) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') } subject { described_class.new(namespace.owner, { namespace_id: namespace.id, path: path, file: file }) } -- cgit v1.2.1 From 9609d1b03508aee363b6a9ad88cdbc414fd25ccc Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 3 Feb 2018 13:30:43 +0000 Subject: Merge branch 'fix-migration-timestamp' into 'master' Update labels migration timestamp to run earlier than the one that removes namespaces to avoid issues See merge request gitlab-org/gitlab-ce!16907 --- ...207150300_remove_project_labels_group_id_copy.rb | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 db/post_migrate/20171207150300_remove_project_labels_group_id_copy.rb diff --git a/db/post_migrate/20171207150300_remove_project_labels_group_id_copy.rb b/db/post_migrate/20171207150300_remove_project_labels_group_id_copy.rb new file mode 100644 index 00000000000..2f339172eeb --- /dev/null +++ b/db/post_migrate/20171207150300_remove_project_labels_group_id_copy.rb @@ -0,0 +1,21 @@ +# Copy of 20180202111106 - this one should run before 20171207150343 to fix issues related to +# the removal of groups with labels. + +class RemoveProjectLabelsGroupIdCopy < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + # rubocop:disable Migration/UpdateColumnInBatches + update_column_in_batches(:labels, :group_id, nil) do |table, query| + query.where(table[:type].eq('ProjectLabel').and(table[:group_id].not_eq(nil))) + end + # rubocop:enable Migration/UpdateColumnInBatches + end + + def down + end +end -- cgit v1.2.1 From aa9864b15646e0fbb9288db06fdf3a82238bd163 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 6 Feb 2018 18:39:50 +0000 Subject: Merge branch 'dm-sidekiq-memory-killer-getcwd' into 'master' Explicitly set cwd in Sidekiq memory killer instead of depending on getcwd See merge request gitlab-org/gitlab-ce!16954 --- lib/gitlab/sidekiq_middleware/memory_killer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/sidekiq_middleware/memory_killer.rb b/lib/gitlab/sidekiq_middleware/memory_killer.rb index 2bfb7caefd9..b89ae2505c9 100644 --- a/lib/gitlab/sidekiq_middleware/memory_killer.rb +++ b/lib/gitlab/sidekiq_middleware/memory_killer.rb @@ -45,7 +45,7 @@ module Gitlab private def get_rss - output, status = Gitlab::Popen.popen(%W(ps -o rss= -p #{pid})) + output, status = Gitlab::Popen.popen(%W(ps -o rss= -p #{pid}), Rails.root.to_s) return 0 unless status.zero? output.to_i -- cgit v1.2.1 From bf7412e48e47c31fe70046285c6109f6a2bb23c5 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 6 Feb 2018 19:17:22 +0000 Subject: Merge branch 'dm-git-hook-popen' into 'master' Only set cwd on the newly spawned process, instead of the current one See merge request gitlab-org/gitlab-ce!16958 --- lib/gitlab/git/hook.rb | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/lib/gitlab/git/hook.rb b/lib/gitlab/git/hook.rb index e29a1f7afa1..24f027d8da4 100644 --- a/lib/gitlab/git/hook.rb +++ b/lib/gitlab/git/hook.rb @@ -82,14 +82,20 @@ module Gitlab end def call_update_hook(gl_id, gl_username, oldrev, newrev, ref) - Dir.chdir(repo_path) do - env = { - 'GL_ID' => gl_id, - 'GL_USERNAME' => gl_username - } - stdout, stderr, status = Open3.capture3(env, path, ref, oldrev, newrev) - [status.success?, (stderr.presence || stdout).gsub(/\R/, "
").html_safe] - end + env = { + 'GL_ID' => gl_id, + 'GL_USERNAME' => gl_username, + 'PWD' => repo_path + } + + options = { + chdir: repo_path + } + + args = [ref, oldrev, newrev] + + stdout, stderr, status = Open3.capture3(env, path, *args, options) + [status.success?, (stderr.presence || stdout).gsub(/\R/, "
").html_safe] end def retrieve_error_message(stderr, stdout) -- cgit v1.2.1 From 0472147942162cca8e9d130c3ba5b6fe4cf4556b Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 8 Feb 2018 10:08:55 -0800 Subject: Fix last batch size equals max batch size error --- .../prepare_untracked_uploads.rb | 2 +- .../prepare_untracked_uploads_spec.rb | 24 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/background_migration/prepare_untracked_uploads.rb b/lib/gitlab/background_migration/prepare_untracked_uploads.rb index 476c46341ae..71d6e2e435d 100644 --- a/lib/gitlab/background_migration/prepare_untracked_uploads.rb +++ b/lib/gitlab/background_migration/prepare_untracked_uploads.rb @@ -88,7 +88,7 @@ module Gitlab end end - yield(paths) + yield(paths) if paths.any? end def build_find_command(search_dir) diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index 8bb9ebe0419..2d0a63dbd62 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -128,6 +128,18 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do expect(untracked_files_for_uploads.count).to eq(5) end end + + context 'when the last batch size exactly matches the max batch size' do + it 'does not raise error' do + stub_const("#{described_class}::FIND_BATCH_SIZE", 5) + + expect do + described_class.new.perform + end.not_to raise_error + + expect(untracked_files_for_uploads.count).to eq(5) + end + end end end @@ -216,6 +228,18 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do expect(untracked_files_for_uploads.count).to eq(5) end end + + context 'when the last batch size exactly matches the max batch size' do + it 'does not raise error' do + stub_const("#{described_class}::FIND_BATCH_SIZE", 5) + + expect do + described_class.new.perform + end.not_to raise_error + + expect(untracked_files_for_uploads.count).to eq(5) + end + end end end -- cgit v1.2.1 From 5199dcd7a15ba8483438f6784191370edcae1ebb Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 8 Feb 2018 10:17:00 -0800 Subject: Fix orphan temp table untracked_files_for_uploads --- .../prepare_untracked_uploads.rb | 11 ++- .../prepare_untracked_uploads_spec.rb | 87 +++++++++------------- 2 files changed, 44 insertions(+), 54 deletions(-) diff --git a/lib/gitlab/background_migration/prepare_untracked_uploads.rb b/lib/gitlab/background_migration/prepare_untracked_uploads.rb index 71d6e2e435d..9fc46f57319 100644 --- a/lib/gitlab/background_migration/prepare_untracked_uploads.rb +++ b/lib/gitlab/background_migration/prepare_untracked_uploads.rb @@ -39,7 +39,11 @@ module Gitlab store_untracked_file_paths - schedule_populate_untracked_uploads_jobs + if UntrackedFile.all.empty? + drop_temp_table + else + schedule_populate_untracked_uploads_jobs + end end private @@ -158,6 +162,11 @@ module Gitlab bulk_queue_background_migration_jobs_by_range( UntrackedFile, FOLLOW_UP_MIGRATION) end + + def drop_temp_table + UntrackedFile.connection.drop_table(:untracked_files_for_uploads, + if_exists: true) + end end end end diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index 2d0a63dbd62..b1f413c99c2 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -8,8 +8,6 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do before do DatabaseCleaner.clean - - drop_temp_table_if_exists end after do @@ -23,25 +21,25 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do end end - it 'ensures the untracked_files_for_uploads table exists' do - expect do - described_class.new.perform - end.to change { ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads) }.from(false).to(true) - end + # E.g. The installation is in use at the time of migration, and someone has + # just uploaded a file + shared_examples 'does not add files in /uploads/tmp' do + let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') } - it 'has a path field long enough for really long paths' do - described_class.new.perform + before do + FileUtils.mkdir(File.dirname(tmp_file)) + FileUtils.touch(tmp_file) + end - component = 'a' * 255 + after do + FileUtils.rm(tmp_file) + end - long_path = [ - 'uploads', - component, # project.full_path - component # filename - ].flatten.join('/') + it 'does not add files from /uploads/tmp' do + described_class.new.perform - record = untracked_files_for_uploads.create!(path: long_path) - expect(record.reload.path.size).to eq(519) + expect(untracked_files_for_uploads.count).to eq(5) + end end context "test bulk insert with ON CONFLICT DO NOTHING or IGNORE" do @@ -69,6 +67,21 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do UploadService.new(project2, uploaded_file, FileUploader).execute end + it 'has a path field long enough for really long paths' do + described_class.new.perform + + component = 'a' * 255 + + long_path = [ + 'uploads', + component, # project.full_path + component # filename + ].flatten.join('/') + + record = untracked_files_for_uploads.create!(path: long_path) + expect(record.reload.path.size).to eq(519) + end + it 'adds unhashed files to the untracked_files_for_uploads table' do described_class.new.perform @@ -109,24 +122,8 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do end end - # E.g. The installation is in use at the time of migration, and someone has - # just uploaded a file context 'when there are files in /uploads/tmp' do - let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') } - - before do - FileUtils.touch(tmp_file) - end - - after do - FileUtils.rm(tmp_file) - end - - it 'does not add files from /uploads/tmp' do - described_class.new.perform - - expect(untracked_files_for_uploads.count).to eq(5) - end + it_behaves_like 'does not add files in /uploads/tmp' end context 'when the last batch size exactly matches the max batch size' do @@ -209,24 +206,8 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do end end - # E.g. The installation is in use at the time of migration, and someone has - # just uploaded a file context 'when there are files in /uploads/tmp' do - let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') } - - before do - FileUtils.touch(tmp_file) - end - - after do - FileUtils.rm(tmp_file) - end - - it 'does not add files from /uploads/tmp' do - described_class.new.perform - - expect(untracked_files_for_uploads.count).to eq(5) - end + it_behaves_like 'does not add files in /uploads/tmp' end context 'when the last batch size exactly matches the max batch size' do @@ -246,10 +227,10 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do # Very new or lightly-used installations that are running this migration # may not have an upload directory because they have no uploads. context 'when no files were ever uploaded' do - it 'does not add to the untracked_files_for_uploads table (and does not raise error)' do + it 'deletes the `untracked_files_for_uploads` table (and does not raise error)' do described_class.new.perform - expect(untracked_files_for_uploads.count).to eq(0) + expect(untracked_files_for_uploads.connection.table_exists?(:untracked_files_for_uploads)).to be_falsey end end end -- cgit v1.2.1 From f44e93e2de5b2b57602aeee21a6cb837d3938905 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 8 Feb 2018 11:10:57 -0800 Subject: Schedule PopulateUntrackedUploads if needed To finish migrating untracked files to uploads for installations that were affected by https://gitlab.com/gitlab-org/gitlab-ce/issues/42881 Or just to delete the temp table if it is empty and left behind. --- ...chedule_populate_untracked_uploads_if_needed.rb | 47 ++++++++++++++++++++++ db/schema.rb | 2 +- 2 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20180208183958_schedule_populate_untracked_uploads_if_needed.rb diff --git a/db/migrate/20180208183958_schedule_populate_untracked_uploads_if_needed.rb b/db/migrate/20180208183958_schedule_populate_untracked_uploads_if_needed.rb new file mode 100644 index 00000000000..e46e793d9d2 --- /dev/null +++ b/db/migrate/20180208183958_schedule_populate_untracked_uploads_if_needed.rb @@ -0,0 +1,47 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class SchedulePopulateUntrackedUploadsIfNeeded < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + FOLLOW_UP_MIGRATION = 'PopulateUntrackedUploads'.freeze + + class UntrackedFile < ActiveRecord::Base + include EachBatch + + self.table_name = 'untracked_files_for_uploads' + end + + def up + if table_exists?(:untracked_files_for_uploads) + process_or_remove_table + end + end + + def down + # nothing + end + + private + + def process_or_remove_table + if UntrackedFile.all.empty? + drop_temp_table + else + schedule_populate_untracked_uploads_jobs + end + end + + def drop_temp_table + drop_table(:untracked_files_for_uploads, if_exists: true) + end + + def schedule_populate_untracked_uploads_jobs + say "Scheduling #{FOLLOW_UP_MIGRATION} background migration jobs since there are rows in untracked_files_for_uploads." + + bulk_queue_background_migration_jobs_by_range( + UntrackedFile, FOLLOW_UP_MIGRATION) + end +end diff --git a/db/schema.rb b/db/schema.rb index f0d94974f41..ac59992fba1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180202111106) do +ActiveRecord::Schema.define(version: 20180208183958) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" -- cgit v1.2.1 From 0a9bed3d5e3dc2ab7395486f1aa8968ff45e6cb9 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 8 Feb 2018 11:17:29 -0800 Subject: Add changelog entry --- changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml diff --git a/changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml b/changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml new file mode 100644 index 00000000000..fddfba94192 --- /dev/null +++ b/changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml @@ -0,0 +1,5 @@ +--- +title: Resolve PrepareUntrackedUploads PostgreSQL syntax error +merge_request: 17019 +author: +type: fixed -- cgit v1.2.1 From 0e109940bcbf563cd0bc2084f4a83241e69e2545 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 14 Feb 2018 13:19:52 -0800 Subject: Fix dir exists spec error --- spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index b1f413c99c2..b21197144a9 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -27,7 +27,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') } before do - FileUtils.mkdir(File.dirname(tmp_file)) + FileUtils.mkdir_p(File.dirname(tmp_file)) FileUtils.touch(tmp_file) end -- cgit v1.2.1 From d6d105f7d71c468d2b176c13414e922d6ca1658d Mon Sep 17 00:00:00 2001 From: Joshua Lambert Date: Thu, 15 Feb 2018 01:01:00 -0500 Subject: Update Prometheus vendor YML for 2.1 support --- vendor/prometheus/values.yaml | 136 ++++++++++++++++++------------------------ 1 file changed, 59 insertions(+), 77 deletions(-) diff --git a/vendor/prometheus/values.yaml b/vendor/prometheus/values.yaml index fdc687b8980..7f8db06471b 100644 --- a/vendor/prometheus/values.yaml +++ b/vendor/prometheus/values.yaml @@ -2,7 +2,7 @@ alertmanager: enabled: false kubeStateMetrics: - enabled: false + enabled: true nodeExporter: enabled: false @@ -10,61 +10,49 @@ nodeExporter: pushgateway: enabled: false +server: + image: + tag: v2.1.0 + serverFiles: - alerts: "" - rules: "" + alerts: {} + rules: {} - prometheus.yml: |- + prometheus.yml: rule_files: - /etc/config/rules - /etc/config/alerts - scrape_configs: - job_name: prometheus static_configs: - targets: - localhost:9090 - - - job_name: 'kubernetes-apiservers' - - kubernetes_sd_configs: - - role: endpoints - - scheme: https - - tls_config: - ca_file: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt - insecure_skip_verify: true - bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token - relabel_configs: - - source_labels: [__meta_kubernetes_namespace, __meta_kubernetes_service_name, __meta_kubernetes_endpoint_port_name] - action: keep - regex: default;kubernetes;https - - - job_name: 'kubernetes-nodes' + - job_name: kubernetes-cadvisor scheme: https tls_config: ca_file: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt insecure_skip_verify: true bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token - kubernetes_sd_configs: - role: node - relabel_configs: - action: labelmap regex: __meta_kubernetes_node_label_(.+) - target_label: __address__ replacement: kubernetes.default.svc:443 - - source_labels: [__meta_kubernetes_node_name] - regex: (.+) + - source_labels: + - __meta_kubernetes_node_name + regex: "(.+)" target_label: __metrics_path__ - replacement: /api/v1/nodes/${1}/proxy/metrics + replacement: "/api/v1/nodes/${1}/proxy/metrics/cadvisor" + metric_relabel_configs: + - source_labels: + - pod_name + target_label: environment + regex: "(.+)-.+-.+" - job_name: 'kubernetes-service-endpoints' - kubernetes_sd_configs: - role: endpoints - relabel_configs: - source_labels: [__meta_kubernetes_service_annotation_prometheus_io_scrape] action: keep @@ -90,66 +78,60 @@ serverFiles: - source_labels: [__meta_kubernetes_service_name] action: replace target_label: kubernetes_name - - - job_name: 'prometheus-pushgateway' - honor_labels: true - - kubernetes_sd_configs: - - role: service - - relabel_configs: - - source_labels: [__meta_kubernetes_service_annotation_prometheus_io_probe] - action: keep - regex: pushgateway - - job_name: 'kubernetes-services' - - metrics_path: /probe - params: - module: [http_2xx] - + - job_name: kubernetes-nodes + scheme: https + tls_config: + ca_file: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt + insecure_skip_verify: true + bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token kubernetes_sd_configs: - - role: service - + - role: node relabel_configs: - - source_labels: [__meta_kubernetes_service_annotation_prometheus_io_probe] - action: keep - regex: true - - source_labels: [__address__] - target_label: __param_target - - target_label: __address__ - replacement: blackbox - - source_labels: [__param_target] - target_label: instance - action: labelmap - regex: __meta_kubernetes_service_label_(.+) - - source_labels: [__meta_kubernetes_namespace] - target_label: kubernetes_namespace - - source_labels: [__meta_kubernetes_service_name] - target_label: kubernetes_name - - - job_name: 'kubernetes-pods' - + regex: __meta_kubernetes_node_label_(.+) + - target_label: __address__ + replacement: kubernetes.default.svc:443 + - source_labels: + - __meta_kubernetes_node_name + regex: "(.+)" + target_label: __metrics_path__ + replacement: "/api/v1/nodes/${1}/proxy/metrics" + metric_relabel_configs: + - source_labels: + - pod_name + target_label: environment + regex: "(.+)-.+-.+" + - job_name: kubernetes-pods + tls_config: + ca_file: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt + insecure_skip_verify: true + bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token kubernetes_sd_configs: - role: pod - relabel_configs: - - source_labels: [__meta_kubernetes_pod_annotation_prometheus_io_scrape] + - source_labels: + - __meta_kubernetes_pod_annotation_prometheus_io_scrape action: keep - regex: true - - source_labels: [__meta_kubernetes_pod_annotation_prometheus_io_path] + regex: 'true' + - source_labels: + - __meta_kubernetes_pod_annotation_prometheus_io_path action: replace target_label: __metrics_path__ - regex: (.+) - - source_labels: [__address__, __meta_kubernetes_pod_annotation_prometheus_io_port] + regex: "(.+)" + - source_labels: + - __address__ + - __meta_kubernetes_pod_annotation_prometheus_io_port action: replace - regex: (.+):(?:\d+);(\d+) - replacement: ${1}:${2} + regex: "([^:]+)(?::[0-9]+)?;([0-9]+)" + replacement: "$1:$2" target_label: __address__ - action: labelmap regex: __meta_kubernetes_pod_label_(.+) - - source_labels: [__meta_kubernetes_namespace] + - source_labels: + - __meta_kubernetes_namespace action: replace target_label: kubernetes_namespace - - source_labels: [__meta_kubernetes_pod_name] + - source_labels: + - __meta_kubernetes_pod_name action: replace - target_label: kubernetes_pod_name + target_label: kubernetes_pod_name \ No newline at end of file -- cgit v1.2.1 From dc737e91f18ee23de2cd1aa0805f4ec5d59a76f1 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 15 Feb 2018 09:23:27 -0600 Subject: Resolve spec/migrations/remove_project_labels_group_id_spec.rb failure --- spec/migrations/remove_project_labels_group_id_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/migrations/remove_project_labels_group_id_spec.rb b/spec/migrations/remove_project_labels_group_id_spec.rb index d80d61af20b..1bb9c3b085a 100644 --- a/spec/migrations/remove_project_labels_group_id_spec.rb +++ b/spec/migrations/remove_project_labels_group_id_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20180202111106_remove_project_labels_group_id.rb') -describe RemoveProjectLabelsGroupId, :delete do +describe RemoveProjectLabelsGroupId, :migration do let(:migration) { described_class.new } let(:group) { create(:group) } let!(:project_label) { create(:label, group_id: group.id) } -- cgit v1.2.1 From fdd986b525d91212f14f35822427cab326b12446 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 15 Feb 2018 09:27:39 +0000 Subject: Merge branch 'fj-37528-error-after-disabling-ldap' into 'master' Fixed user synced attributes metadata after removing current provider Closes #37528 See merge request gitlab-org/gitlab-ce!17054 --- app/models/identity.rb | 9 ++++++ .../fj-37528-error-after-disabling-ldap.yml | 6 ++++ lib/gitlab/ldap/config.rb | 2 +- lib/gitlab/o_auth/user.rb | 8 +++++- spec/lib/gitlab/ldap/config_spec.rb | 8 ++++++ spec/lib/gitlab/o_auth/user_spec.rb | 4 +++ spec/models/identity_spec.rb | 33 ++++++++++++++++++++++ 7 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/fj-37528-error-after-disabling-ldap.yml diff --git a/app/models/identity.rb b/app/models/identity.rb index b3fa7d8176a..2b433e9b988 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -9,6 +9,7 @@ class Identity < ActiveRecord::Base validates :user_id, uniqueness: { scope: :provider } before_save :ensure_normalized_extern_uid, if: :extern_uid_changed? + after_destroy :clear_user_synced_attributes, if: :user_synced_attributes_metadata_from_provider? scope :with_provider, ->(provider) { where(provider: provider) } scope :with_extern_uid, ->(provider, extern_uid) do @@ -34,4 +35,12 @@ class Identity < ActiveRecord::Base self.extern_uid = Identity.normalize_uid(self.provider, self.extern_uid) end + + def user_synced_attributes_metadata_from_provider? + user.user_synced_attributes_metadata&.provider == provider + end + + def clear_user_synced_attributes + user.user_synced_attributes_metadata&.destroy + end end diff --git a/changelogs/unreleased/fj-37528-error-after-disabling-ldap.yml b/changelogs/unreleased/fj-37528-error-after-disabling-ldap.yml new file mode 100644 index 00000000000..1e35f2b537d --- /dev/null +++ b/changelogs/unreleased/fj-37528-error-after-disabling-ldap.yml @@ -0,0 +1,6 @@ +--- +title: Fixed error 500 when removing an identity with synced attributes and visiting + the profile page +merge_request: 17054 +author: +type: fixed diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index 0d9a554fc18..0e8266b67f7 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -15,7 +15,7 @@ module Gitlab end def self.servers - Gitlab.config.ldap.servers.values + Gitlab.config.ldap['servers']&.values || [] end def self.available_servers diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index fff9360ea27..8a7a691736d 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -198,9 +198,11 @@ module Gitlab end def update_profile + clear_user_synced_attributes_metadata + return unless sync_profile_from_provider? || creating_linked_ldap_user? - metadata = gl_user.user_synced_attributes_metadata || gl_user.build_user_synced_attributes_metadata + metadata = gl_user.build_user_synced_attributes_metadata if sync_profile_from_provider? UserSyncedAttributesMetadata::SYNCABLE_ATTRIBUTES.each do |key| @@ -221,6 +223,10 @@ module Gitlab end end + def clear_user_synced_attributes_metadata + gl_user.user_synced_attributes_metadata&.destroy + end + def log Gitlab::AppLogger end diff --git a/spec/lib/gitlab/ldap/config_spec.rb b/spec/lib/gitlab/ldap/config_spec.rb index ca2213cd112..e10837578a8 100644 --- a/spec/lib/gitlab/ldap/config_spec.rb +++ b/spec/lib/gitlab/ldap/config_spec.rb @@ -5,6 +5,14 @@ describe Gitlab::LDAP::Config do let(:config) { described_class.new('ldapmain') } + describe '.servers' do + it 'returns empty array if no server information is available' do + allow(Gitlab.config).to receive(:ldap).and_return('enabled' => false) + + expect(described_class.servers).to eq [] + end + end + describe '#initialize' do it 'requires a provider' do expect { described_class.new }.to raise_error ArgumentError diff --git a/spec/lib/gitlab/o_auth/user_spec.rb b/spec/lib/gitlab/o_auth/user_spec.rb index 45fff4c5787..8370fc655ff 100644 --- a/spec/lib/gitlab/o_auth/user_spec.rb +++ b/spec/lib/gitlab/o_auth/user_spec.rb @@ -712,6 +712,10 @@ describe Gitlab::OAuth::User do it "does not update the user location" do expect(gl_user.location).not_to eq(info_hash[:address][:country]) end + + it 'does not create associated user synced attributes metadata' do + expect(gl_user.user_synced_attributes_metadata).to be_nil + end end end diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb index 7c66c98231b..a5ce245c21d 100644 --- a/spec/models/identity_spec.rb +++ b/spec/models/identity_spec.rb @@ -70,5 +70,38 @@ describe Identity do end end end + + context 'after_destroy' do + let!(:user) { create(:user) } + let(:ldap_identity) { create(:identity, provider: 'ldapmain', extern_uid: 'uid=john smith,ou=people,dc=example,dc=com', user: user) } + let(:ldap_user_synced_attributes) { { provider: 'ldapmain', name_synced: true, email_synced: true } } + let(:other_provider_user_synced_attributes) { { provider: 'other', name_synced: true, email_synced: true } } + + describe 'if user synced attributes metadada provider' do + context 'matches the identity provider ' do + it 'removes the user synced attributes' do + user.create_user_synced_attributes_metadata(ldap_user_synced_attributes) + + expect(user.user_synced_attributes_metadata.provider).to eq 'ldapmain' + + ldap_identity.destroy + + expect(user.reload.user_synced_attributes_metadata).to be_nil + end + end + + context 'does not matche the identity provider' do + it 'does not remove the user synced attributes' do + user.create_user_synced_attributes_metadata(other_provider_user_synced_attributes) + + expect(user.user_synced_attributes_metadata.provider).to eq 'other' + + ldap_identity.destroy + + expect(user.reload.user_synced_attributes_metadata.provider).to eq 'other' + end + end + end + end end end -- cgit v1.2.1 From b197a3d4da005482db833344e194e3ccf1a19f7a Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 15 Feb 2018 16:50:00 +0000 Subject: Merge branch 'fj-fixed-bug-17054' into 'master' Fixed bug with the user synced attributes when the user doesn't exist See merge request gitlab-org/gitlab-ce!17152 --- lib/gitlab/o_auth/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 8a7a691736d..e694be45c9f 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -224,7 +224,7 @@ module Gitlab end def clear_user_synced_attributes_metadata - gl_user.user_synced_attributes_metadata&.destroy + gl_user&.user_synced_attributes_metadata&.destroy end def log -- cgit v1.2.1