From 7ce70dd5a046048ef13eb5f93ece8f149bfbaea3 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 14 Sep 2017 20:25:48 +0100 Subject: Dynamically create offset for sticky bar --- app/assets/javascripts/lib/utils/sticky.js | 30 +++++++++++++++++++++++++----- app/assets/stylesheets/pages/diff.scss | 9 ++------- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/lib/utils/sticky.js b/app/assets/javascripts/lib/utils/sticky.js index 283c0ec0410..f8f0a43f1cc 100644 --- a/app/assets/javascripts/lib/utils/sticky.js +++ b/app/assets/javascripts/lib/utils/sticky.js @@ -1,14 +1,34 @@ -export const isSticky = (el, scrollY, stickyTop) => { +export const createPlaceholder = () => { + const placeholder = document.createElement('div'); + placeholder.classList.add('sticky-placeholder'); + + return placeholder; +}; + +export const isSticky = (el, scrollY, stickyTop, insertPlaceholder) => { const top = Math.floor(el.offsetTop - scrollY); - if (top <= stickyTop) { + if (top <= stickyTop && !el.classList.contains('is-stuck')) { + const placeholder = insertPlaceholder ? createPlaceholder(el) : null; + const heightBefore = el.offsetHeight; + el.classList.add('is-stuck'); - } else { + + if (insertPlaceholder) { + el.parentNode.insertBefore(placeholder, el.nextElementSibling); + + placeholder.style.height = `${heightBefore - el.offsetHeight}px`; + } + } else if (top > stickyTop && el.classList.contains('is-stuck')) { el.classList.remove('is-stuck'); + + if (insertPlaceholder && el.nextElementSibling.classList.contains('sticky-placeholder')) { + el.nextElementSibling.remove(); + } } }; -export default (el) => { +export default (el, insertPlaceholder = true) => { if (!el) return; const computedStyle = window.getComputedStyle(el); @@ -17,7 +37,7 @@ export default (el) => { const stickyTop = parseInt(computedStyle.top, 10); - document.addEventListener('scroll', () => isSticky(el, window.scrollY, stickyTop), { + document.addEventListener('scroll', () => isSticky(el, window.scrollY, stickyTop, insertPlaceholder), { passive: true, }); }; diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 951580ea1fe..5dbf44073b5 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -451,7 +451,7 @@ } .files { - margin-top: -1px; + margin-top: 1px; .diff-file:last-child { margin-bottom: 0; @@ -586,11 +586,6 @@ top: 76px; } - + .files, - + .alert { - margin-top: 1px; - } - &:not(.is-stuck) .diff-stats-additions-deletions-collapsed { display: none; } @@ -608,7 +603,7 @@ + .files, + .alert { - margin-top: 32px; + // margin-top: 32px; } } } -- cgit v1.2.1 From 734bb736258293e213fb0580f1f0b15618a78bc2 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 15 Sep 2017 09:01:58 +0100 Subject: fixed and added specs for removing placeholder element --- spec/javascripts/lib/utils/sticky_spec.js | 40 ++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/spec/javascripts/lib/utils/sticky_spec.js b/spec/javascripts/lib/utils/sticky_spec.js index c3ee3ef9825..5ffc41589e5 100644 --- a/spec/javascripts/lib/utils/sticky_spec.js +++ b/spec/javascripts/lib/utils/sticky_spec.js @@ -4,12 +4,25 @@ describe('sticky', () => { const el = { offsetTop: 0, classList: {}, + parentNode: {}, + nextElementSibling: {}, }; + let isStuck = false; beforeEach(() => { el.offsetTop = 0; - el.classList.add = jasmine.createSpy('spy'); - el.classList.remove = jasmine.createSpy('spy'); + el.classList.add = jasmine.createSpy('classListAdd'); + el.classList.remove = jasmine.createSpy('classListRemove'); + el.classList.contains = jasmine.createSpy('classListContains').and.callFake(() => isStuck); + el.parentNode.insertBefore = jasmine.createSpy('insertBefore'); + el.nextElementSibling.remove = jasmine.createSpy('nextElementSibling'); + el.nextElementSibling.classList = { + contains: jasmine.createSpy('classListContains').and.returnValue(true), + }; + }); + + afterEach(() => { + isStuck = false; }); describe('classList.remove', () => { @@ -21,7 +34,9 @@ describe('sticky', () => { ).not.toHaveBeenCalled(); }); - it('calls classList.remove when not stuck', () => { + it('calls classList.remove when no longer stuck', () => { + isStuck = true; + el.offsetTop = 10; isSticky(el, 0, 0); @@ -29,6 +44,17 @@ describe('sticky', () => { el.classList.remove, ).toHaveBeenCalledWith('is-stuck'); }); + + it('removes placeholder when no longer stuck', () => { + isStuck = true; + + el.offsetTop = 10; + isSticky(el, 0, 0, true); + + expect( + el.nextElementSibling.remove, + ).toHaveBeenCalled(); + }); }); describe('classList.add', () => { @@ -48,5 +74,13 @@ describe('sticky', () => { el.classList.add, ).not.toHaveBeenCalled(); }); + + it('inserts placeholder element when stuck', () => { + isSticky(el, 0, 0, true); + + expect( + el.parentNode.insertBefore, + ).toHaveBeenCalled(); + }); }); }); -- cgit v1.2.1 From da6ff6519818fafe38433599b663a4aeff1007c1 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 21 Sep 2017 08:04:25 +0100 Subject: spec fixes --- app/assets/stylesheets/test.scss | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/assets/stylesheets/test.scss b/app/assets/stylesheets/test.scss index 7d9f3da79c5..06733b7f1a9 100644 --- a/app/assets/stylesheets/test.scss +++ b/app/assets/stylesheets/test.scss @@ -15,3 +15,9 @@ -ms-animation: none !important; animation: none !important; } + +// Disable sticky changes bar for tests +.diff-files-changed { + position: relative !important; + top: 0 !important; +} -- cgit v1.2.1 From 36a917e1a8e5369e349cf467a29226c0f6fa0cac Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Fri, 22 Sep 2017 16:42:05 +0300 Subject: RepoEditor: Implement line and range linking. --- app/assets/javascripts/line_highlighter.js | 283 +++++++++++---------- .../javascripts/repo/components/repo_preview.vue | 14 +- 2 files changed, 151 insertions(+), 146 deletions(-) diff --git a/app/assets/javascripts/line_highlighter.js b/app/assets/javascripts/line_highlighter.js index 7400c22543f..a16d00b5cef 100644 --- a/app/assets/javascripts/line_highlighter.js +++ b/app/assets/javascripts/line_highlighter.js @@ -28,148 +28,149 @@ // // // -(function() { - this.LineHighlighter = (function() { - // CSS class applied to highlighted lines - LineHighlighter.prototype.highlightClass = 'hll'; - - // Internal copy of location.hash so we're not dependent on `location` in tests - LineHighlighter.prototype._hash = ''; - - function LineHighlighter(hash) { - if (hash == null) { - // Initialize a LineHighlighter object - // - // hash - String URL hash for dependency injection in tests - hash = location.hash; - } - this.setHash = this.setHash.bind(this); - this.highlightLine = this.highlightLine.bind(this); - this.clickHandler = this.clickHandler.bind(this); - this.highlightHash = this.highlightHash.bind(this); - this._hash = hash; - this.bindEvents(); - this.highlightHash(); - } - LineHighlighter.prototype.bindEvents = function() { - const $fileHolder = $('.file-holder'); - $fileHolder.on('click', 'a[data-line-number]', this.clickHandler); - $fileHolder.on('highlight:line', this.highlightHash); - }; - - LineHighlighter.prototype.highlightHash = function() { - var range; - if (this._hash !== '') { - range = this.hashToRange(this._hash); - if (range[0]) { - this.highlightRange(range); - $.scrollTo("#L" + range[0], { - // Scroll to the first highlighted line on initial load - // Offset -50 for the sticky top bar, and another -100 for some context - offset: -150 - }); - } - } - }; - - LineHighlighter.prototype.clickHandler = function(event) { - var current, lineNumber, range; - event.preventDefault(); - this.clearHighlight(); - lineNumber = $(event.target).closest('a').data('line-number'); - current = this.hashToRange(this._hash); - if (!(current[0] && event.shiftKey)) { - // If there's no current selection, or there is but Shift wasn't held, - // treat this like a single-line selection. - this.setHash(lineNumber); - return this.highlightLine(lineNumber); - } else if (event.shiftKey) { - if (lineNumber < current[0]) { - range = [lineNumber, current[0]]; - } else { - range = [current[0], lineNumber]; - } - this.setHash(range[0], range[1]); - return this.highlightRange(range); - } - }; - - LineHighlighter.prototype.clearHighlight = function() { - return $("." + this.highlightClass).removeClass(this.highlightClass); - // Unhighlight previously highlighted lines - }; - - // Convert a URL hash String into line numbers - // - // hash - Hash String - // - // Examples: - // - // hashToRange('#L5') # => [5, null] - // hashToRange('#L5-15') # => [5, 15] - // hashToRange('#foo') # => [null, null] - // - // Returns an Array - LineHighlighter.prototype.hashToRange = function(hash) { - var first, last, matches; - // ?L(\d+)(?:-(\d+))?$/) - matches = hash.match(/^#?L(\d+)(?:-(\d+))?$/); - if (matches && matches.length) { - first = parseInt(matches[1], 10); - last = matches[2] ? parseInt(matches[2], 10) : null; - return [first, last]; - } else { - return [null, null]; - } - }; - - // Highlight a single line - // - // lineNumber - Line number to highlight - LineHighlighter.prototype.highlightLine = function(lineNumber) { - return $("#LC" + lineNumber).addClass(this.highlightClass); - }; - - // Highlight all lines within a range - // - // range - Array containing the starting and ending line numbers - LineHighlighter.prototype.highlightRange = function(range) { - var i, lineNumber, ref, ref1, results; - if (range[1]) { - results = []; - for (lineNumber = i = ref = range[0], ref1 = range[1]; ref <= ref1 ? i <= ref1 : i >= ref1; lineNumber = ref <= ref1 ? (i += 1) : (i -= 1)) { - results.push(this.highlightLine(lineNumber)); - } - return results; - } else { - return this.highlightLine(range[0]); - } - }; +const LineHighlighter = function(options = {}) { + options.highlightLineClass = options.highlightLineClass || 'hll'; + options.fileHolderSelector = options.fileHolderSelector || '.file-holder'; + options.scrollFileHolder = options.scrollFileHolder || false; + options.hash = options.hash || location.hash; - // Set the URL hash string - LineHighlighter.prototype.setHash = function(firstLineNumber, lastLineNumber) { - var hash; - if (lastLineNumber) { - hash = "#L" + firstLineNumber + "-" + lastLineNumber; + this.options = options; + this._hash = options.hash; + this.highlightLineClass = options.highlightLineClass; + this.setHash = this.setHash.bind(this); + this.highlightLine = this.highlightLine.bind(this); + this.clickHandler = this.clickHandler.bind(this); + this.highlightHash = this.highlightHash.bind(this); + + this.bindEvents(); + this.highlightHash(); +}; + +LineHighlighter.prototype.bindEvents = function() { + const $fileHolder = $(this.options.fileHolderSelector); + + $fileHolder.on('click', 'a[data-line-number]', this.clickHandler); + $fileHolder.on('highlight:line', this.highlightHash); +}; + +LineHighlighter.prototype.highlightHash = function() { + var range; + + if (this._hash !== '') { + range = this.hashToRange(this._hash); + + if (range[0]) { + this.highlightRange(range); + const lineSelector = `#L${range[0]}`; + const scrollOptions = { + // Scroll to the first highlighted line on initial load + // Offset -50 for the sticky top bar, and another -100 for some context + offset: -150 + }; + if (this.options.scrollFileHolder) { + $(this.options.fileHolderSelector).scrollTo(lineSelector, scrollOptions); } else { - hash = "#L" + firstLineNumber; + $.scrollTo(lineSelector, scrollOptions); } - this._hash = hash; - return this.__setLocationHash__(hash); - }; - - // Make the actual hash change in the browser - // - // This method is stubbed in tests. - LineHighlighter.prototype.__setLocationHash__ = function(value) { - return history.pushState({ - url: value - // We're using pushState instead of assigning location.hash directly to - // prevent the page from scrolling on the hashchange event - }, document.title, value); - }; - - return LineHighlighter; - })(); -}).call(window); + } + } +}; + +LineHighlighter.prototype.clickHandler = function(event) { + var current, lineNumber, range; + event.preventDefault(); + this.clearHighlight(); + lineNumber = $(event.target).closest('a').data('line-number'); + current = this.hashToRange(this._hash); + if (!(current[0] && event.shiftKey)) { + // If there's no current selection, or there is but Shift wasn't held, + // treat this like a single-line selection. + this.setHash(lineNumber); + return this.highlightLine(lineNumber); + } else if (event.shiftKey) { + if (lineNumber < current[0]) { + range = [lineNumber, current[0]]; + } else { + range = [current[0], lineNumber]; + } + this.setHash(range[0], range[1]); + return this.highlightRange(range); + } +}; + +LineHighlighter.prototype.clearHighlight = function() { + return $("." + this.highlightLineClass).removeClass(this.highlightLineClass); +}; + +// Convert a URL hash String into line numbers +// +// hash - Hash String +// +// Examples: +// +// hashToRange('#L5') # => [5, null] +// hashToRange('#L5-15') # => [5, 15] +// hashToRange('#foo') # => [null, null] +// +// Returns an Array +LineHighlighter.prototype.hashToRange = function(hash) { + var first, last, matches; + // ?L(\d+)(?:-(\d+))?$/) + matches = hash.match(/^#?L(\d+)(?:-(\d+))?$/); + if (matches && matches.length) { + first = parseInt(matches[1], 10); + last = matches[2] ? parseInt(matches[2], 10) : null; + return [first, last]; + } else { + return [null, null]; + } +}; + +// Highlight a single line +// +// lineNumber - Line number to highlight +LineHighlighter.prototype.highlightLine = function(lineNumber) { + return $("#LC" + lineNumber).addClass(this.highlightLineClass); +}; + +// Highlight all lines within a range +// +// range - Array containing the starting and ending line numbers +LineHighlighter.prototype.highlightRange = function(range) { + var i, lineNumber, ref, ref1, results; + if (range[1]) { + results = []; + for (lineNumber = i = ref = range[0], ref1 = range[1]; ref <= ref1 ? i <= ref1 : i >= ref1; lineNumber = ref <= ref1 ? (i += 1) : (i -= 1)) { + results.push(this.highlightLine(lineNumber)); + } + return results; + } else { + return this.highlightLine(range[0]); + } +}; + +// Set the URL hash string +LineHighlighter.prototype.setHash = function(firstLineNumber, lastLineNumber) { + var hash; + if (lastLineNumber) { + hash = "#L" + firstLineNumber + "-" + lastLineNumber; + } else { + hash = "#L" + firstLineNumber; + } + this._hash = hash; + return this.__setLocationHash__(hash); +}; + +// Make the actual hash change in the browser +// +// This method is stubbed in tests. +LineHighlighter.prototype.__setLocationHash__ = function(value) { + return history.pushState({ + url: value + // We're using pushState instead of assigning location.hash directly to + // prevent the page from scrolling on the hashchange event + }, document.title, value); +}; + +window.LineHighlighter = LineHighlighter; diff --git a/app/assets/javascripts/repo/components/repo_preview.vue b/app/assets/javascripts/repo/components/repo_preview.vue index 2200754cbef..0d9d132f766 100644 --- a/app/assets/javascripts/repo/components/repo_preview.vue +++ b/app/assets/javascripts/repo/components/repo_preview.vue @@ -1,23 +1,27 @@ + diff --git a/app/assets/javascripts/cycle_analytics/components/stage_code_component.js b/app/assets/javascripts/cycle_analytics/components/stage_code_component.js deleted file mode 100644 index 7c32a38fbe7..00000000000 --- a/app/assets/javascripts/cycle_analytics/components/stage_code_component.js +++ /dev/null @@ -1,51 +0,0 @@ -/* eslint-disable no-param-reassign */ - -import Vue from 'vue'; -import userAvatarImage from '../../vue_shared/components/user_avatar/user_avatar_image.vue'; - -const global = window.gl || (window.gl = {}); -global.cycleAnalytics = global.cycleAnalytics || {}; - -global.cycleAnalytics.StageCodeComponent = Vue.extend({ - props: { - items: Array, - stage: Object, - }, - components: { - userAvatarImage, - }, - template: ` -
-
- {{ stage.description }} - -
- -
- `, -}); diff --git a/app/assets/javascripts/cycle_analytics/components/stage_code_component.vue b/app/assets/javascripts/cycle_analytics/components/stage_code_component.vue new file mode 100644 index 00000000000..e4d62b649e5 --- /dev/null +++ b/app/assets/javascripts/cycle_analytics/components/stage_code_component.vue @@ -0,0 +1,47 @@ + + diff --git a/app/assets/javascripts/cycle_analytics/components/stage_component.vue b/app/assets/javascripts/cycle_analytics/components/stage_component.vue new file mode 100644 index 00000000000..ab730af8f5b --- /dev/null +++ b/app/assets/javascripts/cycle_analytics/components/stage_component.vue @@ -0,0 +1,53 @@ + + + diff --git a/app/assets/javascripts/cycle_analytics/components/stage_issue_component.js b/app/assets/javascripts/cycle_analytics/components/stage_issue_component.js deleted file mode 100644 index 5f4a0ac8590..00000000000 --- a/app/assets/javascripts/cycle_analytics/components/stage_issue_component.js +++ /dev/null @@ -1,52 +0,0 @@ -/* eslint-disable no-param-reassign */ -import Vue from 'vue'; -import userAvatarImage from '../../vue_shared/components/user_avatar/user_avatar_image.vue'; - -const global = window.gl || (window.gl = {}); -global.cycleAnalytics = global.cycleAnalytics || {}; - -global.cycleAnalytics.StageIssueComponent = Vue.extend({ - props: { - items: Array, - stage: Object, - }, - components: { - userAvatarImage, - }, - template: ` -
-
- {{ stage.description }} - -
- -
- `, -}); diff --git a/app/assets/javascripts/cycle_analytics/components/stage_plan_component.js b/app/assets/javascripts/cycle_analytics/components/stage_plan_component.js deleted file mode 100644 index 11fee5410d9..00000000000 --- a/app/assets/javascripts/cycle_analytics/components/stage_plan_component.js +++ /dev/null @@ -1,53 +0,0 @@ -/* eslint-disable no-param-reassign */ -import Vue from 'vue'; -import userAvatarImage from '../../vue_shared/components/user_avatar/user_avatar_image.vue'; -import iconCommit from '../svg/icon_commit.svg'; - -const global = window.gl || (window.gl = {}); -global.cycleAnalytics = global.cycleAnalytics || {}; - -global.cycleAnalytics.StagePlanComponent = Vue.extend({ - props: { - items: Array, - stage: Object, - }, - components: { - userAvatarImage, - }, - data() { - return { iconCommit }; - }, - template: ` -
-
- {{ stage.description }} - -
- -
- `, -}); diff --git a/app/assets/javascripts/cycle_analytics/components/stage_plan_component.vue b/app/assets/javascripts/cycle_analytics/components/stage_plan_component.vue new file mode 100644 index 00000000000..152c086a606 --- /dev/null +++ b/app/assets/javascripts/cycle_analytics/components/stage_plan_component.vue @@ -0,0 +1,56 @@ + + + diff --git a/app/assets/javascripts/cycle_analytics/components/stage_production_component.js b/app/assets/javascripts/cycle_analytics/components/stage_production_component.js deleted file mode 100644 index b7ba9360f70..00000000000 --- a/app/assets/javascripts/cycle_analytics/components/stage_production_component.js +++ /dev/null @@ -1,52 +0,0 @@ -/* eslint-disable no-param-reassign */ -import Vue from 'vue'; -import userAvatarImage from '../../vue_shared/components/user_avatar/user_avatar_image.vue'; - -const global = window.gl || (window.gl = {}); -global.cycleAnalytics = global.cycleAnalytics || {}; - -global.cycleAnalytics.StageProductionComponent = Vue.extend({ - props: { - items: Array, - stage: Object, - }, - components: { - userAvatarImage, - }, - template: ` -
-
- {{ stage.description }} - -
- -
- `, -}); diff --git a/app/assets/javascripts/cycle_analytics/components/stage_review_component.js b/app/assets/javascripts/cycle_analytics/components/stage_review_component.js deleted file mode 100644 index f41a0d0e4ff..00000000000 --- a/app/assets/javascripts/cycle_analytics/components/stage_review_component.js +++ /dev/null @@ -1,62 +0,0 @@ -/* eslint-disable no-param-reassign */ -import Vue from 'vue'; -import userAvatarImage from '../../vue_shared/components/user_avatar/user_avatar_image.vue'; - -const global = window.gl || (window.gl = {}); -global.cycleAnalytics = global.cycleAnalytics || {}; - -global.cycleAnalytics.StageReviewComponent = Vue.extend({ - props: { - items: Array, - stage: Object, - }, - components: { - userAvatarImage, - }, - template: ` -
-
- {{ stage.description }} - -
- -
- `, -}); diff --git a/app/assets/javascripts/cycle_analytics/components/stage_review_component.vue b/app/assets/javascripts/cycle_analytics/components/stage_review_component.vue new file mode 100644 index 00000000000..9e66b690404 --- /dev/null +++ b/app/assets/javascripts/cycle_analytics/components/stage_review_component.vue @@ -0,0 +1,62 @@ + + diff --git a/app/assets/javascripts/cycle_analytics/components/stage_staging_component.js b/app/assets/javascripts/cycle_analytics/components/stage_staging_component.js deleted file mode 100644 index d7c906c9d39..00000000000 --- a/app/assets/javascripts/cycle_analytics/components/stage_staging_component.js +++ /dev/null @@ -1,53 +0,0 @@ -/* eslint-disable no-param-reassign */ -import Vue from 'vue'; -import userAvatarImage from '../../vue_shared/components/user_avatar/user_avatar_image.vue'; -import iconBranch from '../svg/icon_branch.svg'; - -const global = window.gl || (window.gl = {}); -global.cycleAnalytics = global.cycleAnalytics || {}; - -global.cycleAnalytics.StageStagingComponent = Vue.extend({ - props: { - items: Array, - stage: Object, - }, - data() { - return { iconBranch }; - }, - components: { - userAvatarImage, - }, - template: ` -
-
- {{ stage.description }} - -
- -
- `, -}); diff --git a/app/assets/javascripts/cycle_analytics/components/stage_staging_component.vue b/app/assets/javascripts/cycle_analytics/components/stage_staging_component.vue new file mode 100644 index 00000000000..2787b5ea47b --- /dev/null +++ b/app/assets/javascripts/cycle_analytics/components/stage_staging_component.vue @@ -0,0 +1,55 @@ + + diff --git a/app/assets/javascripts/cycle_analytics/components/stage_test_component.js b/app/assets/javascripts/cycle_analytics/components/stage_test_component.js deleted file mode 100644 index 78cc97eea0b..00000000000 --- a/app/assets/javascripts/cycle_analytics/components/stage_test_component.js +++ /dev/null @@ -1,49 +0,0 @@ -/* eslint-disable no-param-reassign */ -import Vue from 'vue'; -import iconBuildStatus from '../svg/icon_build_status.svg'; -import iconBranch from '../svg/icon_branch.svg'; - -const global = window.gl || (window.gl = {}); -global.cycleAnalytics = global.cycleAnalytics || {}; - -global.cycleAnalytics.StageTestComponent = Vue.extend({ - props: { - items: Array, - stage: Object, - }, - data() { - return { iconBuildStatus, iconBranch }; - }, - template: ` -
-
- {{ stage.description }} - -
- -
- `, -}); diff --git a/app/assets/javascripts/cycle_analytics/components/stage_test_component.vue b/app/assets/javascripts/cycle_analytics/components/stage_test_component.vue new file mode 100644 index 00000000000..9c3d39ce011 --- /dev/null +++ b/app/assets/javascripts/cycle_analytics/components/stage_test_component.vue @@ -0,0 +1,54 @@ + + diff --git a/app/assets/javascripts/cycle_analytics/components/total_time_component.js b/app/assets/javascripts/cycle_analytics/components/total_time_component.js deleted file mode 100644 index d5e6167b2a8..00000000000 --- a/app/assets/javascripts/cycle_analytics/components/total_time_component.js +++ /dev/null @@ -1,25 +0,0 @@ -/* eslint-disable no-param-reassign */ - -import Vue from 'vue'; - -const global = window.gl || (window.gl = {}); -global.cycleAnalytics = global.cycleAnalytics || {}; - -global.cycleAnalytics.TotalTimeComponent = Vue.extend({ - props: { - time: Object, - }, - template: ` - - - - - `, -}); diff --git a/app/assets/javascripts/cycle_analytics/components/total_time_component.vue b/app/assets/javascripts/cycle_analytics/components/total_time_component.vue new file mode 100644 index 00000000000..9941b997b3f --- /dev/null +++ b/app/assets/javascripts/cycle_analytics/components/total_time_component.vue @@ -0,0 +1,29 @@ + + diff --git a/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js b/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js index 5f1221c4c49..8002b0b23c9 100644 --- a/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js +++ b/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js @@ -3,15 +3,14 @@ import Vue from 'vue'; import Cookies from 'js-cookie'; import Translate from '../vue_shared/translate'; -import LimitWarningComponent from './components/limit_warning_component'; -import './components/stage_code_component'; -import './components/stage_issue_component'; -import './components/stage_plan_component'; -import './components/stage_production_component'; -import './components/stage_review_component'; -import './components/stage_staging_component'; -import './components/stage_test_component'; -import './components/total_time_component'; +import limitWarningComponent from './components/limit_warning_component.vue'; +import stageCodeComponent from './components/stage_code_component.vue'; +import stagePlanComponent from './components/stage_plan_component.vue'; +import stageComponent from './components/stage_component.vue'; +import stageReviewComponent from './components/stage_review_component.vue'; +import stageStagingComponent from './components/stage_staging_component.vue'; +import stageTestComponent from './components/stage_test_component.vue'; +import totalTime from './components/total_time_component.vue'; import CycleAnalyticsService from './cycle_analytics_service'; import CycleAnalyticsStore from './cycle_analytics_store'; @@ -47,13 +46,13 @@ $(() => { }, }, components: { - 'stage-issue-component': gl.cycleAnalytics.StageIssueComponent, - 'stage-plan-component': gl.cycleAnalytics.StagePlanComponent, - 'stage-code-component': gl.cycleAnalytics.StageCodeComponent, - 'stage-test-component': gl.cycleAnalytics.StageTestComponent, - 'stage-review-component': gl.cycleAnalytics.StageReviewComponent, - 'stage-staging-component': gl.cycleAnalytics.StageStagingComponent, - 'stage-production-component': gl.cycleAnalytics.StageProductionComponent, + 'stage-issue-component': stageComponent, + 'stage-plan-component': stagePlanComponent, + 'stage-code-component': stageCodeComponent, + 'stage-test-component': stageTestComponent, + 'stage-review-component': stageReviewComponent, + 'stage-staging-component': stageStagingComponent, + 'stage-production-component': stageComponent, }, created() { this.fetchCycleAnalyticsData(); @@ -136,6 +135,6 @@ $(() => { }); // Register global components - Vue.component('limit-warning', LimitWarningComponent); - Vue.component('total-time', gl.cycleAnalytics.TotalTimeComponent); + Vue.component('limit-warning', limitWarningComponent); + Vue.component('total-time', totalTime); }); diff --git a/spec/javascripts/cycle_analytics/limit_warning_component_spec.js b/spec/javascripts/cycle_analytics/limit_warning_component_spec.js index 2fb9eb0ca85..13e9fe00a00 100644 --- a/spec/javascripts/cycle_analytics/limit_warning_component_spec.js +++ b/spec/javascripts/cycle_analytics/limit_warning_component_spec.js @@ -1,6 +1,6 @@ import Vue from 'vue'; import Translate from '~/vue_shared/translate'; -import limitWarningComp from '~/cycle_analytics/components/limit_warning_component'; +import limitWarningComp from '~/cycle_analytics/components/limit_warning_component.vue'; Vue.use(Translate); -- cgit v1.2.1 From a019369409e38ee6cb36e5782cefd8c944b12e53 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 26 Sep 2017 10:49:53 +0200 Subject: Expose avatar_url when requesting list of projects from API with simple=true --- changelogs/unreleased/dm-simple-project-avatar-url.yml | 5 +++++ lib/api/entities.rb | 11 ++++++----- spec/requests/api/environments_spec.rb | 1 + spec/requests/api/projects_spec.rb | 1 + spec/requests/api/v3/projects_spec.rb | 1 + 5 files changed, 14 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/dm-simple-project-avatar-url.yml diff --git a/changelogs/unreleased/dm-simple-project-avatar-url.yml b/changelogs/unreleased/dm-simple-project-avatar-url.yml new file mode 100644 index 00000000000..e517345f5d2 --- /dev/null +++ b/changelogs/unreleased/dm-simple-project-avatar-url.yml @@ -0,0 +1,5 @@ +--- +title: Expose avatar_url when requesting list of projects from API with simple=true +merge_request: +author: +type: added diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 71253f72533..9776a9dd74b 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -89,6 +89,9 @@ module API expose :ssh_url_to_repo, :http_url_to_repo, :web_url expose :name, :name_with_namespace expose :path, :path_with_namespace + expose :avatar_url do |project, options| + project.avatar_url(only_path: false) + end expose :star_count, :forks_count expose :created_at, :last_activity_at end @@ -146,9 +149,7 @@ module API expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? } expose :import_status expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] } - expose :avatar_url do |user, options| - user.avatar_url(only_path: false) - end + expose :open_issues_count, if: lambda { |project, options| project.feature_available?(:issues, options[:current_user]) } expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] } expose :public_builds, as: :public_jobs @@ -193,8 +194,8 @@ module API class Group < Grape::Entity expose :id, :name, :path, :description, :visibility expose :lfs_enabled?, as: :lfs_enabled - expose :avatar_url do |user, options| - user.avatar_url(only_path: false) + expose :avatar_url do |group, options| + group.avatar_url(only_path: false) end expose :web_url expose :request_access_enabled diff --git a/spec/requests/api/environments_spec.rb b/spec/requests/api/environments_spec.rb index 2361809e0e1..f8cd529a06c 100644 --- a/spec/requests/api/environments_spec.rb +++ b/spec/requests/api/environments_spec.rb @@ -20,6 +20,7 @@ describe API::Environments do path path_with_namespace star_count forks_count created_at last_activity_at + avatar_url ) get api("/projects/#{project.id}/environments", user) diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 508df990952..18f6f7df1fa 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -193,6 +193,7 @@ describe API::Projects do path path_with_namespace star_count forks_count created_at last_activity_at + avatar_url ) get api('/projects?simple=true', user) diff --git a/spec/requests/api/v3/projects_spec.rb b/spec/requests/api/v3/projects_spec.rb index cae2c3118da..e5282c3311f 100644 --- a/spec/requests/api/v3/projects_spec.rb +++ b/spec/requests/api/v3/projects_spec.rb @@ -89,6 +89,7 @@ describe API::V3::Projects do path path_with_namespace star_count forks_count created_at last_activity_at + avatar_url ) get v3_api('/projects?simple=true', user) -- cgit v1.2.1 From 2e221b8117fb98fc64d1ffb98196c77d4e172aa4 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 26 Sep 2017 11:52:30 +0100 Subject: Fix sidebar mobile toggle button border --- app/assets/stylesheets/framework/new-nav.scss | 5 ----- app/assets/stylesheets/framework/new-sidebar.scss | 7 +++++++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/app/assets/stylesheets/framework/new-nav.scss b/app/assets/stylesheets/framework/new-nav.scss index c3badec59c9..d4b3fb238d5 100644 --- a/app/assets/stylesheets/framework/new-nav.scss +++ b/app/assets/stylesheets/framework/new-nav.scss @@ -317,11 +317,6 @@ header.navbar-gitlab-new { align-self: center; color: $gl-text-color-secondary; - @media (max-width: $screen-xs-max) { - padding-left: 17px; - border-left: 1px solid $gl-text-color-quaternary; - } - .avatar-tile { margin-right: 4px; border: 1px solid $border-color; diff --git a/app/assets/stylesheets/framework/new-sidebar.scss b/app/assets/stylesheets/framework/new-sidebar.scss index 9c404b7e542..3f1cb97aebc 100644 --- a/app/assets/stylesheets/framework/new-sidebar.scss +++ b/app/assets/stylesheets/framework/new-sidebar.scss @@ -461,6 +461,13 @@ $new-sidebar-collapsed-width: 50px; font-size: 18px; } } + + @media (max-width: $screen-xs-max) { + + .breadcrumbs-links { + padding-left: 17px; + border-left: 1px solid $gl-text-color-quaternary; + } + } } @media (max-width: $screen-xs-max) { -- cgit v1.2.1 From da15b38850d8d345a17862754285c34a2fbf08b6 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 13:00:29 +0200 Subject: Add specs for pipeline builder abilities validator --- app/services/ci/create_pipeline_service.rb | 3 -- .../ci/pipeline/chain/validate/abilities_spec.rb | 39 ++++++++++++++++------ 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 0bf38776afc..af1784fe1dc 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -46,9 +46,6 @@ module Ci private - def process_pipeline_sequence - end - def commit @commit ||= project.commit(origin_sha || origin_ref) end diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb index 9e217102886..e6decde475a 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb @@ -1,22 +1,41 @@ require 'spec_helper' describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do - describe '#allowed_to_create?' do - let(:user) { create(:user) } - let(:project) { create(:project, :repository) } - let(:ref) { 'master' } + set(:project) { create(:project, :repository) } + set(:user) { create(:user) } + + let(:pipeline) do + build_stubbed(:ci_pipeline, ref: ref, project: project) + end + + let(:command) do + double('command', project: project, current_user: user) + end + + let(:step) { described_class.new(pipeline, command) } - let(:pipeline) do - build_stubbed(:ci_pipeline, ref: ref, project: project) + let(:ref) { 'master' } + + context 'when users has no ability to run a pipeline' do + before do + step.perform! end - let(:command) do - double('command', project: project, current_user: user) + it 'adds an error about insufficient permissions' do + expect(pipeline.errors.to_a) + .to include /Insufficient permissions/ end - subject do - described_class.new(pipeline, command).allowed_to_create? + it 'breaks the pipeline builder chain' do + expect(step.break?).to eq true end + end + + context 'when user has ability to create a pipeline' do + end + + describe '#allowed_to_create?' do + subject { step.allowed_to_create? } context 'when user is a developer' do before do -- cgit v1.2.1 From 1a8777c8d775b951384fbe4b8cf050b26b247d00 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 13:02:57 +0200 Subject: Fix coding style offenses in pipeline chain classes --- app/services/ci/create_pipeline_service.rb | 1 - lib/gitlab/ci/pipeline/chain/create.rb | 4 +--- lib/gitlab/ci/pipeline/chain/sequence.rb | 2 +- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index af1784fe1dc..df5b32d97ca 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -28,7 +28,6 @@ module Ci project: project, current_user: current_user) - sequence = Gitlab::Ci::Pipeline::Chain::Sequence .new(pipeline, command, SEQUENCE) diff --git a/lib/gitlab/ci/pipeline/chain/create.rb b/lib/gitlab/ci/pipeline/chain/create.rb index 8bd658b3069..d5e17a123df 100644 --- a/lib/gitlab/ci/pipeline/chain/create.rb +++ b/lib/gitlab/ci/pipeline/chain/create.rb @@ -9,9 +9,7 @@ module Gitlab ::Ci::Pipeline.transaction do pipeline.save! - if @command.seeds_block - @command.seeds_block.call(pipeline) - end + @command.seeds_block&.call(pipeline) ::Ci::CreatePipelineStagesService .new(project, current_user) diff --git a/lib/gitlab/ci/pipeline/chain/sequence.rb b/lib/gitlab/ci/pipeline/chain/sequence.rb index e8d1ab36883..c80d583939c 100644 --- a/lib/gitlab/ci/pipeline/chain/sequence.rb +++ b/lib/gitlab/ci/pipeline/chain/sequence.rb @@ -13,7 +13,7 @@ module Gitlab end def build! - @sequence.each do |step| + @sequence.each do |step| step.perform! break if step.break? -- cgit v1.2.1 From f696b04cc88bd38395caf8ccb4003c28f38a9c47 Mon Sep 17 00:00:00 2001 From: Mehdi Lahmam Date: Sun, 13 Aug 2017 21:15:04 +0200 Subject: Expose last pipeline details in API response when getting a single commit Closes #35692. --- ...se-last-pipeline-details-in-api-for-single-commit.yml | 5 +++++ doc/api/commits.md | 6 ++++++ lib/api/entities.rb | 1 + .../api/schemas/public_api/v4/commit/detail.json | 11 +++++++++-- .../api/schemas/public_api/v4/pipeline/basic.json | 16 ++++++++++++++++ spec/requests/api/commits_spec.rb | 5 +++++ 6 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/expose-last-pipeline-details-in-api-for-single-commit.yml create mode 100644 spec/fixtures/api/schemas/public_api/v4/pipeline/basic.json diff --git a/changelogs/unreleased/expose-last-pipeline-details-in-api-for-single-commit.yml b/changelogs/unreleased/expose-last-pipeline-details-in-api-for-single-commit.yml new file mode 100644 index 00000000000..d16e052cd92 --- /dev/null +++ b/changelogs/unreleased/expose-last-pipeline-details-in-api-for-single-commit.yml @@ -0,0 +1,5 @@ +--- +title: Expose last pipeline details in API response when getting a single commit +merge_request: 13521 +author: Mehdi Lahmam (@mehlah) +type: added diff --git a/doc/api/commits.md b/doc/api/commits.md index 2a78553782f..5a4a8d888b3 100644 --- a/doc/api/commits.md +++ b/doc/api/commits.md @@ -181,6 +181,12 @@ Example response: "parent_ids": [ "ae1d9fb46aa2b07ee9836d49862ec4e2c46fbbba" ], + "last_pipeline" : { + "id": 8, + "ref": "master", + "sha": "2dc6aa325a317eda67812f05600bdf0fcdc70ab0" + "status": "created" + } "stats": { "additions": 15, "deletions": 10, diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 71253f72533..085d1edc0fc 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -234,6 +234,7 @@ module API class RepoCommitDetail < RepoCommit expose :stats, using: Entities::RepoCommitStats expose :status + expose :last_pipeline, using: 'API::Entities::PipelineBasic' end class RepoBranch < Grape::Entity diff --git a/spec/fixtures/api/schemas/public_api/v4/commit/detail.json b/spec/fixtures/api/schemas/public_api/v4/commit/detail.json index b7b2535c204..88a3cad62f6 100644 --- a/spec/fixtures/api/schemas/public_api/v4/commit/detail.json +++ b/spec/fixtures/api/schemas/public_api/v4/commit/detail.json @@ -5,11 +5,18 @@ { "required" : [ "stats", - "status" + "status", + "last_pipeline" ], "properties": { "stats": { "$ref": "../commit_stats.json" }, - "status": { "type": ["string", "null"] } + "status": { "type": ["string", "null"] }, + "last_pipeline": { + "oneOf": [ + { "type": "null" }, + { "$ref": "../pipeline/basic.json" } + ] + } } } ] diff --git a/spec/fixtures/api/schemas/public_api/v4/pipeline/basic.json b/spec/fixtures/api/schemas/public_api/v4/pipeline/basic.json new file mode 100644 index 00000000000..0d127dc5297 --- /dev/null +++ b/spec/fixtures/api/schemas/public_api/v4/pipeline/basic.json @@ -0,0 +1,16 @@ +{ + "type": "object", + "required" : [ + "id", + "sha", + "ref", + "status" + ], + "properties" : { + "id": { "type": "integer" }, + "sha": { "type": "string" }, + "ref": { "type": "string" }, + "status": { "type": "string" } + }, + "additionalProperties": false +} diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index f663719d28c..94462b4572d 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -491,6 +491,7 @@ describe API::Commits do expect(json_response['stats']['deletions']).to eq(commit.stats.deletions) expect(json_response['stats']['total']).to eq(commit.stats.total) expect(json_response['status']).to be_nil + expect(json_response['last_pipeline']).to be_nil end context 'when ref does not exist' do @@ -573,6 +574,10 @@ describe API::Commits do expect(response).to have_http_status(200) expect(response).to match_response_schema('public_api/v4/commit/detail') expect(json_response['status']).to eq('created') + expect(json_response['last_pipeline']['id']).to eq(pipeline.id) + expect(json_response['last_pipeline']['ref']).to eq(pipeline.ref) + expect(json_response['last_pipeline']['sha']).to eq(pipeline.sha) + expect(json_response['last_pipeline']['status']).to eq(pipeline.status) end context 'when pipeline succeeds' do -- cgit v1.2.1 From 2432d5bd9ec49ce5a6287ee57ef701c740dff7f9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 13:22:28 +0200 Subject: Add specs for builder chain step that skipps pipelines --- lib/gitlab/ci/pipeline/chain/skip.rb | 2 +- spec/lib/gitlab/ci/pipeline/chain/skip_spec.rb | 85 ++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 spec/lib/gitlab/ci/pipeline/chain/skip_spec.rb diff --git a/lib/gitlab/ci/pipeline/chain/skip.rb b/lib/gitlab/ci/pipeline/chain/skip.rb index 3f86275ae15..9a72de87bab 100644 --- a/lib/gitlab/ci/pipeline/chain/skip.rb +++ b/lib/gitlab/ci/pipeline/chain/skip.rb @@ -24,7 +24,7 @@ module Gitlab def commit_message_skips_ci? return false unless @pipeline.git_commit_message - @skipped ||= @pipeline.git_commit_message =~ SKIP_PATTERN + @skipped ||= !!(@pipeline.git_commit_message =~ SKIP_PATTERN) end end end diff --git a/spec/lib/gitlab/ci/pipeline/chain/skip_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/skip_spec.rb new file mode 100644 index 00000000000..32bd5de829b --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/chain/skip_spec.rb @@ -0,0 +1,85 @@ +require 'spec_helper' + +describe Gitlab::Ci::Pipeline::Chain::Skip do + set(:project) { create(:project) } + set(:user) { create(:user) } + set(:pipeline) { create(:ci_pipeline, project: project) } + + let(:command) do + double('command', project: project, + current_user: user, + ignore_skip_ci: false, + save_incompleted: true) + end + + let(:step) { described_class.new(pipeline, command) } + + context 'when pipeline has been skipped by a user' do + before do + allow(pipeline).to receive(:git_commit_message) + .and_return('commit message [ci skip]') + + step.perform! + end + + it 'should break the chain' do + expect(step.break?).to be true + end + + it 'skips the pipeline' do + expect(pipeline.reload).to be_skipped + end + end + + context 'when pipeline has not been skipped' do + before do + step.perform! + end + + it 'should not break the chain' do + expect(step.break?).to be false + end + + it 'should not skip a pipeline chain' do + expect(pipeline.reload).not_to be_skipped + end + end + + context 'when [ci skip] should be ignored' do + let(:command) do + double('command', project: project, + current_user: user, + ignore_skip_ci: true) + end + + it 'does not break the chain' do + step.perform! + + expect(step.break?).to be false + end + end + + context 'when pipeline should be skipped but not persisted' do + let(:command) do + double('command', project: project, + current_user: user, + ignore_skip_ci: false, + save_incompleted: false) + end + + before do + allow(pipeline).to receive(:git_commit_message) + .and_return('commit message [ci skip]') + + step.perform! + end + + it 'breaks the chain' do + expect(step.break?).to be true + end + + it 'does not skip pipeline' do + expect(pipeline.reload).not_to be_skipped + end + end +end -- cgit v1.2.1 From 652ecff91be036382ecac6059093bfae80522c81 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 13:34:53 +0200 Subject: Add specs for builder chain that persist a pipeline --- spec/lib/gitlab/ci/pipeline/chain/create_spec.rb | 66 ++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 spec/lib/gitlab/ci/pipeline/chain/create_spec.rb diff --git a/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb new file mode 100644 index 00000000000..f54e2326b06 --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb @@ -0,0 +1,66 @@ +require 'spec_helper' + +describe Gitlab::Ci::Pipeline::Chain::Create do + set(:project) { create(:project) } + set(:user) { create(:user) } + + let(:pipeline) do + build(:ci_pipeline_with_one_job, project: project, + ref: 'master') + end + + let(:command) do + double('command', project: project, + current_user: user, + seeds_block: nil) + end + + let(:step) { described_class.new(pipeline, command) } + + before do + step.perform! + end + + context 'when pipeline is ready to be saved' do + it 'saves a pipeline' do + expect(pipeline).to be_persisted + end + + it 'does not break the chain' do + expect(step.break?).to be false + end + + it 'creates stages' do + expect(pipeline.reload.stages).to be_one + end + end + + context 'when pipeline has validation errors' do + let(:pipeline) do + build(:ci_pipeline, project: project, ref: nil) + end + + it 'breaks the chain' do + expect(step.break?).to be true + end + + it 'appends validation error' do + expect(pipeline.errors.to_a) + .to include /Failed to persist the pipeline/ + end + end + + context 'when there is a seed block present' do + let(:seeds) { spy('pipeline seeds') } + + let(:command) do + double('command', project: project, + current_user: user, + seeds_block: seeds) + end + + it 'executes the block' do + expect(seeds).to have_received(:call).with(pipeline) + end + end +end -- cgit v1.2.1 From 39f05fd85ed53e2045f615a55baa3158f5eb20cc Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 13:40:37 +0200 Subject: Add specs for pipeline builder repository validator --- .../ci/pipeline/chain/validate/repository_spec.rb | 60 ++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb new file mode 100644 index 00000000000..bb356efe9ad --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb @@ -0,0 +1,60 @@ +require 'spec_helper' + +describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do + set(:project) { create(:project, :repository) } + set(:user) { create(:user) } + + let(:command) do + double('command', project: project, current_user: user) + end + + let!(:step) { described_class.new(pipeline, command) } + + before do + step.perform! + end + + context 'when pipeline ref and sha exists' do + let(:pipeline) do + build_stubbed(:ci_pipeline, ref: 'master', sha: '123', project: project) + end + + it 'does not break the chain' do + expect(step.break?).to be false + end + + it 'does not append pipeline errors' do + expect(pipeline.errors).to be_empty + end + end + + context 'when pipeline ref does not exist' do + let(:pipeline) do + build_stubbed(:ci_pipeline, ref: 'something', project: project) + end + + it 'breaks the chain' do + expect(step.break?).to be true + end + + it 'adds an error about missing ref' do + expect(pipeline.errors.to_a) + .to include 'Reference not found' + end + end + + context 'when pipeline does not have SHA set' do + let(:pipeline) do + build_stubbed(:ci_pipeline, ref: 'master', sha: nil, project: project) + end + + it 'breaks the chain' do + expect(step.break?).to be true + end + + it 'adds an error about missing SHA' do + expect(pipeline.errors.to_a) + .to include 'Commit not found' + end + end +end -- cgit v1.2.1 From 6a9cfdde02b33b0a53d9f2b0f1bb3ee0e092a46d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 14:04:28 +0200 Subject: Add specs for pipeline builder that validates config --- .../ci/pipeline/chain/validate/config_spec.rb | 127 +++++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb new file mode 100644 index 00000000000..d5e885b7409 --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb @@ -0,0 +1,127 @@ +require 'spec_helper' + +describe Gitlab::Ci::Pipeline::Chain::Validate::Config do + set(:project) { create(:project) } + set(:user) { create(:user) } + + let(:command) do + double('command', project: project, + current_user: user, + save_incompleted: true) + end + + let!(:step) { described_class.new(pipeline, command) } + + before do + step.perform! + end + + context 'when pipeline has no YAML configuration' do + let(:pipeline) do + build_stubbed(:ci_pipeline, project: project) + end + + it 'appends errors about missing configuration' do + expect(pipeline.errors.to_a) + .to include 'Missing .gitlab-ci.yml file' + end + + it 'breaks the chain' do + expect(step.break?).to be true + end + end + + context 'when YAML configuration contains errors' do + let(:pipeline) do + build(:ci_pipeline, project: project, config: 'invalid YAML') + end + + it 'appends errors about YAML errors' do + expect(pipeline.errors.to_a) + .to include 'Invalid configuration format' + end + + it 'breaks the chain' do + expect(step.break?).to be true + end + + context 'when saving incomplete pipeline is allowed' do + let(:command) do + double('command', project: project, + current_user: user, + save_incompleted: true) + end + + it 'fails the pipeline' do + expect(pipeline.reload).to be_failed + end + end + + context 'when saving incomplete pipeline is not allowed' do + let(:command) do + double('command', project: project, + current_user: user, + save_incompleted: false) + end + + it 'does not drop pipeline' do + expect(pipeline).not_to be_failed + expect(pipeline).not_to be_persisted + end + end + end + + context 'when pipeline has no stages / jobs' do + let(:config) do + { rspec: { + script: 'ls', + only: ['something'] + } + } + end + + let(:pipeline) do + build(:ci_pipeline, project: project, config: config) + end + + it 'appends an error about missing stages' do + expect(pipeline.errors.to_a) + .to include 'No stages / jobs for this pipeline.' + end + + it 'breaks the chain' do + expect(step.break?).to be true + end + end + + context 'when pipeline contains configuration validation errors' do + let(:config) { { rspec: { } } } + + let(:pipeline) do + build(:ci_pipeline, project: project, config: config) + end + + it 'appends configuration validation errors to pipeline errors' do + expect(pipeline.errors.to_a) + .to include "jobs:rspec config can't be blank" + end + + it 'breaks the chain' do + expect(step.break?).to be true + end + end + + context 'when pipeline is correct and complete' do + let(:pipeline) do + build(:ci_pipeline_with_one_job, project: project) + end + + it 'does not invalidate the pipeline' do + expect(pipeline).to be_valid + end + + it 'does not break the chain' do + expect(step.break?).to be false + end + end +end -- cgit v1.2.1 From 53cad500433b69c77628ce020587fd5c9227690c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 14:13:08 +0200 Subject: Add missing tests for pipeline chain access validator --- .../lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb index e6decde475a..0bbdd23f4d6 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb @@ -32,6 +32,19 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do end context 'when user has ability to create a pipeline' do + before do + project.add_developer(user) + + step.perform! + end + + it 'does not invalidate the pipeline' do + expect(pipeline).to be_valid + end + + it 'does not break the chain' do + expect(step.break?).to eq false + end end describe '#allowed_to_create?' do -- cgit v1.2.1 From f6bd832f3fb5a833422c7a08deda844f6e78ab93 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 14:15:05 +0200 Subject: Fix some code style offenses in pipeline chain classes --- spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb index d5e885b7409..3740df88f42 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb @@ -76,8 +76,7 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Config do { rspec: { script: 'ls', only: ['something'] - } - } + } } end let(:pipeline) do @@ -95,7 +94,7 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Config do end context 'when pipeline contains configuration validation errors' do - let(:config) { { rspec: { } } } + let(:config) { { rspec: {} } } let(:pipeline) do build(:ci_pipeline, project: project, config: config) -- cgit v1.2.1 From 835bdcb88e3eff70b33d27b6ca42e7d1970eac35 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 14:24:40 +0200 Subject: Add specs for pipeline chain builder sequence class --- spec/lib/gitlab/ci/pipeline/chain/sequence_spec.rb | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 spec/lib/gitlab/ci/pipeline/chain/sequence_spec.rb diff --git a/spec/lib/gitlab/ci/pipeline/chain/sequence_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/sequence_spec.rb new file mode 100644 index 00000000000..e165e0fac2a --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/chain/sequence_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' + +describe Gitlab::Ci::Pipeline::Chain::Sequence do + set(:project) { create(:project) } + set(:user) { create(:user) } + + let(:pipeline) { build_stubbed(:ci_pipeline) } + let(:command) { double('command' ) } + let(:first_step) { spy('first step') } + let(:second_step) { spy('second step') } + let(:sequence) { [first_step, second_step] } + + subject do + described_class.new(pipeline, command, sequence) + end + + context 'when one of steps breaks the chain' do + before do + allow(first_step).to receive(:break?).and_return(true) + end + + it 'does not process the second step' do + subject.build! do |pipeline, sequence| + expect(sequence).not_to be_complete + end + + expect(second_step).not_to have_received(:perform!) + end + + it 'returns a pipeline object' do + expect(subject.build!).to eq pipeline + end + end + + context 'when all chains are executed correctly' do + before do + sequence.each do |step| + allow(step).to receive(:break?).and_return(false) + end + end + + it 'iterates through entire sequence' do + subject.build! do |pipeline, sequence| + expect(sequence).to be_complete + end + + expect(first_step).to have_received(:perform!) + expect(second_step).to have_received(:perform!) + end + + it 'returns a pipeline object' do + expect(subject.build!).to eq pipeline + end + end +end -- cgit v1.2.1 From 057a8b709346a89e2ccdfe6e9b352ce5f93e71c7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 26 Sep 2017 15:01:16 +0200 Subject: Add test for head pipeline assignment when skipped Closes gitlab-org/gitlab-ce#34415 --- spec/services/ci/create_pipeline_service_spec.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 6ee75b8fc23..eb6e683cc23 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -133,6 +133,26 @@ describe Ci::CreatePipelineService do expect(merge_request.reload.head_pipeline).to eq head_pipeline end end + + context 'when pipeline has been skipped' do + before do + allow_any_instance_of(Ci::Pipeline) + .to receive(:git_commit_message) + .and_return('some commit [ci skip]') + end + + it 'updates merge request head pipeline' do + merge_request = create(:merge_request, source_branch: 'master', + target_branch: 'feature', + source_project: project) + + head_pipeline = execute_service + + expect(head_pipeline).to be_skipped + expect(head_pipeline).to be_persisted + expect(merge_request.reload.head_pipeline).to eq head_pipeline + end + end end context 'auto-cancel enabled' do -- cgit v1.2.1 From 6528d52afedf7c9fd9db4ae9e101060cfcbe53d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 15 Sep 2017 16:02:46 +0200 Subject: Backport doc change from latest upstream merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2906 Signed-off-by: Rémy Coutable --- .../project/repository/gpg_signed_commits/index.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/doc/user/project/repository/gpg_signed_commits/index.md b/doc/user/project/repository/gpg_signed_commits/index.md index dfe43c6b691..29e04a0ccf0 100644 --- a/doc/user/project/repository/gpg_signed_commits/index.md +++ b/doc/user/project/repository/gpg_signed_commits/index.md @@ -113,25 +113,25 @@ started: 1. Use the following command to list the private GPG key you just created: ``` - gpg --list-secret-keys --keyid-format 0xLONG mr@robot.sh + gpg --list-secret-keys --keyid-format LONG mr@robot.sh ``` Replace `mr@robot.sh` with the email address you entered above. 1. Copy the GPG key ID that starts with `sec`. In the following example, that's - `0x30F2B65B9246B6CA`: + `30F2B65B9246B6CA`: ``` - sec rsa4096/0x30F2B65B9246B6CA 2017-08-18 [SC] + sec rsa4096/30F2B65B9246B6CA 2017-08-18 [SC] D5E4F29F3275DC0CDA8FFC8730F2B65B9246B6CA uid [ultimate] Mr. Robot - ssb rsa4096/0xB7ABC0813E4028C0 2017-08-18 [E] + ssb rsa4096/B7ABC0813E4028C0 2017-08-18 [E] ``` 1. Export the public key of that ID (replace your key ID from the previous step): ``` - gpg --armor --export 0x30F2B65B9246B6CA + gpg --armor --export 30F2B65B9246B6CA ``` 1. Finally, copy the public key and [add it in your profile settings](#adding-a-gpg-key-to-your-account) @@ -167,28 +167,28 @@ key to use. 1. Use the following command to list the private GPG key you just created: ``` - gpg --list-secret-keys --keyid-format 0xLONG mr@robot.sh + gpg --list-secret-keys --keyid-format LONG mr@robot.sh ``` Replace `mr@robot.sh` with the email address you entered above. 1. Copy the GPG key ID that starts with `sec`. In the following example, that's - `0x30F2B65B9246B6CA`: + `30F2B65B9246B6CA`: ``` - sec rsa4096/0x30F2B65B9246B6CA 2017-08-18 [SC] + sec rsa4096/30F2B65B9246B6CA 2017-08-18 [SC] D5E4F29F3275DC0CDA8FFC8730F2B65B9246B6CA uid [ultimate] Mr. Robot - ssb rsa4096/0xB7ABC0813E4028C0 2017-08-18 [E] + ssb rsa4096/B7ABC0813E4028C0 2017-08-18 [E] ``` 1. Tell Git to use that key to sign the commits: ``` - git config --global user.signingkey 0x30F2B65B9246B6CA + git config --global user.signingkey 30F2B65B9246B6CA ``` - Replace `0x30F2B65B9246B6CA` with your GPG key ID. + Replace `30F2B65B9246B6CA` with your GPG key ID. ## Signing commits -- cgit v1.2.1 From 951a5cca6202ed64de0687205d13b2b03346e514 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 15 Sep 2017 18:20:29 +0200 Subject: Backport part of c777bb91fd7 and 4074cb3b7c16 from EE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/controllers/concerns/issuable_collections.rb | 2 +- app/finders/issuable_finder.rb | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 8921d55c3d0..3181f517087 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -106,7 +106,7 @@ module IssuableCollections # @filter_params[:authorized_only] = true end - @filter_params + @filter_params.permit(IssuableFinder::VALID_PARAMS) end def set_default_state diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 0a2e3c709d9..673cd36046d 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -25,6 +25,10 @@ class IssuableFinder NONE = '0'.freeze + SCALAR_PARAMS = %i(scope state group_id project_id milestone_title assignee_id search label_name sort assignee_username author_id author_username authorized_only due_date iids non_archived weight).freeze + ARRAY_PARAMS = { label_name: [], iids: [], assignee_username: [] }.freeze + VALID_PARAMS = (SCALAR_PARAMS + [ARRAY_PARAMS]).freeze + attr_accessor :current_user, :params def initialize(current_user, params = {}) -- cgit v1.2.1 From 966b34098cbf90b9edab113ba434e0f476b1c022 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 26 Sep 2017 15:23:47 +0100 Subject: Improves i18n for Auto Devops callout --- app/views/shared/_auto_devops_callout.html.haml | 10 +++++----- changelogs/unreleased/3523-i18n-autodevops.yml | 5 +++++ 2 files changed, 10 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/3523-i18n-autodevops.yml diff --git a/app/views/shared/_auto_devops_callout.html.haml b/app/views/shared/_auto_devops_callout.html.haml index 2f09c2fec87..88e1ed301d4 100644 --- a/app/views/shared/_auto_devops_callout.html.haml +++ b/app/views/shared/_auto_devops_callout.html.haml @@ -6,10 +6,10 @@ .svg-container = custom_icon('icon_autodevops') .user-callout-copy - %h4= _('Auto DevOps (Beta)') - %p= _('Auto DevOps can be activated for this project. It will automatically build, test, and deploy your application based on a predefined CI/CD configuration.') + %h4= s_('AutoDevOps|Auto DevOps (Beta)') + %p= s_('AutoDevOps|Auto DevOps can be activated for this project. It will automatically build, test, and deploy your application based on a predefined CI/CD configuration.') %p - #{s_('AutoDevOps|Learn more in the')} - = link_to _('Auto DevOps documentation'), help_page_path('topics/autodevops/index.md'), target: '_blank', rel: 'noopener noreferrer' + - link = link_to(s_('AutoDevOps|Auto DevOps documentation'), help_page_path('topics/autodevops/index.md'), target: '_blank', rel: 'noopener noreferrer') + = s_('AutoDevOps|Learn more in the %{link}').html_safe % { link: link } - = link_to _('Enable in settings'), project_settings_ci_cd_path(@project, anchor: 'js-general-pipeline-settings'), class: 'btn btn-primary js-close-callout' + = link_to s_('AutoDevOps|Enable in settings'), project_settings_ci_cd_path(@project, anchor: 'js-general-pipeline-settings'), class: 'btn btn-primary js-close-callout' diff --git a/changelogs/unreleased/3523-i18n-autodevops.yml b/changelogs/unreleased/3523-i18n-autodevops.yml new file mode 100644 index 00000000000..10cb22b42a0 --- /dev/null +++ b/changelogs/unreleased/3523-i18n-autodevops.yml @@ -0,0 +1,5 @@ +--- +title: Improves i18n for Auto Devops callout +merge_request: +author: +type: other -- cgit v1.2.1 From a4168719abefe9cfa6c417d73e0833823a92fa08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 26 Sep 2017 16:18:45 +0200 Subject: Remove `weight` from IssuableFinder::SCALAR_PARAMS and improve the array formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/finders/issuable_finder.rb | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 673cd36046d..24c07f3dc70 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -25,8 +25,26 @@ class IssuableFinder NONE = '0'.freeze - SCALAR_PARAMS = %i(scope state group_id project_id milestone_title assignee_id search label_name sort assignee_username author_id author_username authorized_only due_date iids non_archived weight).freeze + SCALAR_PARAMS = %i[ + assignee_id + assignee_username + author_id + author_username + authorized_only + due_date + group_id + iids + label_name + milestone_title + non_archived + project_id + scope + search + sort + state + ].freeze ARRAY_PARAMS = { label_name: [], iids: [], assignee_username: [] }.freeze + VALID_PARAMS = (SCALAR_PARAMS + [ARRAY_PARAMS]).freeze attr_accessor :current_user, :params -- cgit v1.2.1 From 60053c12014db906afdc481638de9f746e663a40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 21 Sep 2017 15:29:36 +0200 Subject: Fetch 100 results when calling the GitHub API in Github::Import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- lib/github/import.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/github/import.rb b/lib/github/import.rb index 9354e142d3d..3074a9fea0b 100644 --- a/lib/github/import.rb +++ b/lib/github/import.rb @@ -115,7 +115,7 @@ module Github url = "/repos/#{repo}/labels" while url - response = Github::Client.new(options).get(url) + response = Github::Client.new(options).get(url, per_page: 100) response.body.each do |raw| begin @@ -139,7 +139,7 @@ module Github url = "/repos/#{repo}/milestones" while url - response = Github::Client.new(options).get(url, state: :all) + response = Github::Client.new(options).get(url, state: :all, per_page: 100) response.body.each do |raw| begin @@ -168,7 +168,7 @@ module Github url = "/repos/#{repo}/pulls" while url - response = Github::Client.new(options).get(url, state: :all, sort: :created, direction: :asc) + response = Github::Client.new(options).get(url, state: :all, sort: :created, direction: :asc, per_page: 100) response.body.each do |raw| pull_request = Github::Representation::PullRequest.new(raw, options.merge(project: project)) @@ -224,7 +224,7 @@ module Github url = "/repos/#{repo}/issues" while url - response = Github::Client.new(options).get(url, state: :all, sort: :created, direction: :asc) + response = Github::Client.new(options).get(url, state: :all, sort: :created, direction: :asc, per_page: 100) response.body.each { |raw| populate_issue(raw) } @@ -276,7 +276,7 @@ module Github def fetch_comments(noteable, type, url, klass = Note) while url - comments = Github::Client.new(options).get(url) + comments = Github::Client.new(options).get(url, per_page: 100) ActiveRecord::Base.no_touching do comments.body.each do |raw| @@ -308,7 +308,7 @@ module Github url = "/repos/#{repo}/releases" while url - response = Github::Client.new(options).get(url) + response = Github::Client.new(options).get(url, per_page: 100) response.body.each do |raw| representation = Github::Representation::Release.new(raw) -- cgit v1.2.1 From e484a06472890424c9f7174a3e80d5a5e43eae57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 21 Sep 2017 15:30:12 +0200 Subject: Retrieve PR comments only when we know there are any MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Pull Request resource doesn't include the `comments` field, while the Issue resource does. And since we're looping through all issues anyway, we can freely check if the issue is a PR and has comments and in this case only fetch comments for it. That means if you have 1000 PRs but only 200 with comments, you will do 200 API requests instead of 1000. :notbad: Signed-off-by: Rémy Coutable --- lib/github/client.rb | 3 ++- lib/github/import.rb | 41 ++++++++++++++++++++++------------------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/lib/github/client.rb b/lib/github/client.rb index 9c476df7d46..29bd9c1f39e 100644 --- a/lib/github/client.rb +++ b/lib/github/client.rb @@ -1,6 +1,7 @@ module Github class Client TIMEOUT = 60 + DEFAULT_PER_PAGE = 100 attr_reader :connection, :rate_limit @@ -20,7 +21,7 @@ module Github exceed, reset_in = rate_limit.get sleep reset_in if exceed - Github::Response.new(connection.get(url, query)) + Github::Response.new(connection.get(url, { per_page: DEFAULT_PER_PAGE }.merge(query))) end private diff --git a/lib/github/import.rb b/lib/github/import.rb index 3074a9fea0b..fdc82ff530d 100644 --- a/lib/github/import.rb +++ b/lib/github/import.rb @@ -115,7 +115,7 @@ module Github url = "/repos/#{repo}/labels" while url - response = Github::Client.new(options).get(url, per_page: 100) + response = Github::Client.new(options).get(url) response.body.each do |raw| begin @@ -139,7 +139,7 @@ module Github url = "/repos/#{repo}/milestones" while url - response = Github::Client.new(options).get(url, state: :all, per_page: 100) + response = Github::Client.new(options).get(url, state: :all) response.body.each do |raw| begin @@ -168,7 +168,7 @@ module Github url = "/repos/#{repo}/pulls" while url - response = Github::Client.new(options).get(url, state: :all, sort: :created, direction: :asc, per_page: 100) + response = Github::Client.new(options).get(url, state: :all, sort: :created, direction: :asc) response.body.each do |raw| pull_request = Github::Representation::PullRequest.new(raw, options.merge(project: project)) @@ -202,13 +202,8 @@ module Github merge_request.save!(validate: false) merge_request.merge_request_diffs.create - # Fetch review comments review_comments_url = "/repos/#{repo}/pulls/#{pull_request.iid}/comments" fetch_comments(merge_request, :review_comment, review_comments_url, LegacyDiffNote) - - # Fetch comments - comments_url = "/repos/#{repo}/issues/#{pull_request.iid}/comments" - fetch_comments(merge_request, :comment, comments_url) rescue => e error(:pull_request, pull_request.url, e.message) ensure @@ -224,7 +219,7 @@ module Github url = "/repos/#{repo}/issues" while url - response = Github::Client.new(options).get(url, state: :all, sort: :created, direction: :asc, per_page: 100) + response = Github::Client.new(options).get(url, state: :all, sort: :created, direction: :asc) response.body.each { |raw| populate_issue(raw) } @@ -241,12 +236,17 @@ module Github # for both features, like manipulating assignees, labels # and milestones, are provided within the Issues API. if representation.pull_request? - return unless representation.has_labels? + return if !representation.has_labels? && !representation.has_comments? merge_request = MergeRequest.find_by!(target_project_id: project.id, iid: representation.iid) - merge_request.update_attribute(:label_ids, label_ids(representation.labels)) + + if representation.has_labels? + merge_request.update_attribute(:label_ids, label_ids(representation.labels)) + end + + fetch_comments_conditionally(merge_request, representation) else - return if Issue.where(iid: representation.iid, project_id: project.id).exists? + return if Issue.exists?(iid: representation.iid, project_id: project.id) author_id = user_id(representation.author, project.creator_id) issue = Issue.new @@ -263,20 +263,23 @@ module Github issue.updated_at = representation.updated_at issue.save!(validate: false) - # Fetch comments - if representation.has_comments? - comments_url = "/repos/#{repo}/issues/#{issue.iid}/comments" - fetch_comments(issue, :comment, comments_url) - end + fetch_comments_conditionally(issue, representation) end rescue => e error(:issue, representation.url, e.message) end end + def fetch_comments_conditionally(issuable, representation) + if representation.has_comments? + comments_url = "/repos/#{repo}/issues/#{issuable.iid}/comments" + fetch_comments(issuable, :comment, comments_url) + end + end + def fetch_comments(noteable, type, url, klass = Note) while url - comments = Github::Client.new(options).get(url, per_page: 100) + comments = Github::Client.new(options).get(url) ActiveRecord::Base.no_touching do comments.body.each do |raw| @@ -308,7 +311,7 @@ module Github url = "/repos/#{repo}/releases" while url - response = Github::Client.new(options).get(url, per_page: 100) + response = Github::Client.new(options).get(url) response.body.each do |raw| representation = Github::Representation::Release.new(raw) -- cgit v1.2.1 From 62120acf1d8a7b3abecfff71900ed423c1fdf473 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 25 Sep 2017 15:37:30 +0200 Subject: Add a spec for Github::Client and revert an `if !` to `unless` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- lib/github/import.rb | 2 +- spec/lib/github/client_spec.rb | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 spec/lib/github/client_spec.rb diff --git a/lib/github/import.rb b/lib/github/import.rb index fdc82ff530d..f5f62dc8b6f 100644 --- a/lib/github/import.rb +++ b/lib/github/import.rb @@ -236,7 +236,7 @@ module Github # for both features, like manipulating assignees, labels # and milestones, are provided within the Issues API. if representation.pull_request? - return if !representation.has_labels? && !representation.has_comments? + return unless representation.has_labels? || representation.has_comments? merge_request = MergeRequest.find_by!(target_project_id: project.id, iid: representation.iid) diff --git a/spec/lib/github/client_spec.rb b/spec/lib/github/client_spec.rb new file mode 100644 index 00000000000..b846096fe25 --- /dev/null +++ b/spec/lib/github/client_spec.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +describe Github::Client do + let(:connection) { spy } + let(:rate_limit) { double(get: [false, 1]) } + let(:client) { described_class.new({}) } + let(:results) { double } + let(:response) { double } + + before do + allow(Faraday).to receive(:new).and_return(connection) + allow(Github::RateLimit).to receive(:new).with(connection).and_return(rate_limit) + end + + describe '#get' do + before do + allow(Github::Response).to receive(:new).with(results).and_return(response) + end + + it 'uses a default per_page param' do + expect(connection).to receive(:get).with('/foo', per_page: 100).and_return(results) + + expect(client.get('/foo')).to eq(response) + end + + context 'with per_page given' do + it 'overwrites the default per_page' do + expect(connection).to receive(:get).with('/foo', per_page: 30).and_return(results) + + expect(client.get('/foo', per_page: 30)).to eq(response) + end + end + end +end -- cgit v1.2.1 From d70b7a490dae32435c5c906bc6f923676c52a9ea Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Thu, 21 Sep 2017 15:10:33 +0100 Subject: find_user users helper method no longer overrides find_user API helper method. --- ...er-method-from-users-endpoint-overrides-api-helper-method.yml | 5 +++++ lib/api/users.rb | 4 ++-- spec/requests/api/users_spec.rb | 9 +++++++++ 3 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/37467-helper-method-from-users-endpoint-overrides-api-helper-method.yml diff --git a/changelogs/unreleased/37467-helper-method-from-users-endpoint-overrides-api-helper-method.yml b/changelogs/unreleased/37467-helper-method-from-users-endpoint-overrides-api-helper-method.yml new file mode 100644 index 00000000000..1984ec6e81c --- /dev/null +++ b/changelogs/unreleased/37467-helper-method-from-users-endpoint-overrides-api-helper-method.yml @@ -0,0 +1,5 @@ +--- +title: find_user Users helper method no longer overrides find_user API helper method. +merge_request: 14418 +author: +type: fixed diff --git a/lib/api/users.rb b/lib/api/users.rb index bdebda58d3f..77ac24ec68d 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -11,7 +11,7 @@ module API end helpers do - def find_user(params) + def find_user_by_id(params) id = params[:user_id] || params[:id] User.find_by(id: id) || not_found!('User') end @@ -430,7 +430,7 @@ module API resource :impersonation_tokens do helpers do def finder(options = {}) - user = find_user(params) + user = find_user_by_id(params) PersonalAccessTokensFinder.new({ user: user, impersonation: true }.merge(options)) end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 5b306ec6cbf..c30188b1b41 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -125,6 +125,15 @@ describe API::Users do end context "when admin" do + context 'when sudo is defined' do + it 'does not return 500' do + admin_personal_access_token = create(:personal_access_token, user: admin).token + get api("/users?private_token=#{admin_personal_access_token}&sudo=#{user.id}", admin) + + expect(response).to have_http_status(:success) + end + end + it "returns an array of users" do get api("/users", admin) -- cgit v1.2.1 From e7ad2bf5f104098e356facf2cc53c046cf624a39 Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Tue, 26 Sep 2017 21:19:28 +0000 Subject: Make issue boards sidebar full height --- app/assets/javascripts/boards/boards_bundle.js | 6 +++--- app/assets/stylesheets/framework/variables.scss | 1 + app/assets/stylesheets/pages/boards.scss | 26 +++++++++++-------------- app/assets/stylesheets/pages/issuable.scss | 4 ++-- app/views/shared/boards/_show.html.haml | 8 ++++---- 5 files changed, 21 insertions(+), 24 deletions(-) diff --git a/app/assets/javascripts/boards/boards_bundle.js b/app/assets/javascripts/boards/boards_bundle.js index ea00efe4b46..815248f38ee 100644 --- a/app/assets/javascripts/boards/boards_bundle.js +++ b/app/assets/javascripts/boards/boards_bundle.js @@ -77,9 +77,6 @@ $(() => { }); Store.rootPath = this.boardsEndpoint; - this.filterManager = new FilteredSearchBoards(Store.filter, true); - this.filterManager.setup(); - // Listen for updateTokens event eventHub.$on('updateTokens', this.updateTokens); }, @@ -87,6 +84,9 @@ $(() => { eventHub.$off('updateTokens', this.updateTokens); }, mounted () { + this.filterManager = new FilteredSearchBoards(Store.filter, true); + this.filterManager.setup(); + Store.disabled = this.disabled; gl.boardService.all() .then(response => response.json()) diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index ca514add9cd..e8bb42f4a8c 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -7,6 +7,7 @@ $gutter_inner_width: 250px; $sidebar-transition-duration: .15s; $sidebar-breakpoint: 1024px; $default-transition-duration: .15s; +$right-sidebar-transition-duration: .3s; /* * Color schema diff --git a/app/assets/stylesheets/pages/boards.scss b/app/assets/stylesheets/pages/boards.scss index 700be173039..3305a482a0d 100644 --- a/app/assets/stylesheets/pages/boards.scss +++ b/app/assets/stylesheets/pages/boards.scss @@ -55,6 +55,15 @@ .boards-app { position: relative; + + @media (min-width: $screen-sm-min) { + transition: width $right-sidebar-transition-duration; + width: 100%; + + &.is-compact { + width: calc(100% - #{$gutter_width}); + } + } } .boards-app-loading { @@ -78,11 +87,6 @@ height: calc(100vh - 222px); // scss-lint:enable DuplicateProperty min-height: 475px; - transition: width .2s; - - &.is-compact { - width: calc(100% - 290px); - } } } @@ -412,14 +416,6 @@ .page-with-layout-nav.page-with-sub-nav .issue-boards-sidebar, .page-with-new-sidebar.page-with-sidebar .issue-boards-sidebar { - position: absolute; - - &.right-sidebar { - top: 0; - bottom: 0; - height: 100%; - } - .issuable-sidebar-header { position: relative; } @@ -457,8 +453,8 @@ .right-sidebar.right-sidebar-expanded { &.boards-sidebar-slide-enter-active, &.boards-sidebar-slide-leave-active { - transition: width .2s, - padding .2s; + transition: width $right-sidebar-transition-duration, + padding $right-sidebar-transition-duration; } &.boards-sidebar-slide-enter, diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 3ddf3d8ea7a..7eb28354e6d 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -223,14 +223,14 @@ top: $new-navbar-height; bottom: 0; right: 0; - transition: width .3s; + transition: width $right-sidebar-transition-duration; background: $gray-light; z-index: 200; overflow: hidden; .issuable-sidebar { width: calc(100% + 100px); - height: calc(100% - #{$new-navbar-height}); + height: 100%; overflow-y: scroll; overflow-x: hidden; -webkit-overflow-scrolling: touch; diff --git a/app/views/shared/boards/_show.html.haml b/app/views/shared/boards/_show.html.haml index 7e22c678e81..ee8ad8e3999 100644 --- a/app/views/shared/boards/_show.html.haml +++ b/app/views/shared/boards/_show.html.haml @@ -12,11 +12,11 @@ %script#js-board-template{ type: "text/x-template" }= render "shared/boards/components/board" %script#js-board-modal-filter{ type: "text/x-template" }= render "shared/issuable/search_bar", type: :boards_modal -.hidden-xs.hidden-sm - = render 'shared/issuable/search_bar', type: :boards +#board-app.boards-app{ "v-cloak" => true, data: board_data, ":class" => "{ 'is-compact': detailIssueVisible }" } + .hidden-xs.hidden-sm + = render 'shared/issuable/search_bar', type: :boards -#board-app.boards-app{ "v-cloak" => true, data: board_data } - .boards-list{ ":class" => "{ 'is-compact': detailIssueVisible }" } + .boards-list .boards-app-loading.text-center{ "v-if" => "loading" } = icon("spinner spin") %board{ "v-cloak" => true, -- cgit v1.2.1 From b8fb11df73b417411ee555c178a932a456d274bf Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Tue, 26 Sep 2017 17:00:40 -0500 Subject: Cleanup diff-notes-collapse css --- app/assets/stylesheets/pages/diff.scss | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index ed9d5e98467..e4bd783c8bc 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -535,7 +535,6 @@ } .diff-notes-collapse { - position: relative; width: 19px; height: 19px; padding: 0; @@ -543,11 +542,7 @@ z-index: 100; svg { - position: absolute; - left: 50%; - top: 50%; - margin-left: -5.5px; - margin-top: -5.5px; + vertical-align: text-top; } path { -- cgit v1.2.1 From cae3417381222c527077be5330ef1b2222d31103 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Tue, 26 Sep 2017 19:40:04 -0300 Subject: Don't enforce gitaly request limits for distinct calls --- lib/gitlab/gitaly_client.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index 955d2307f88..3f10951f49e 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -165,7 +165,13 @@ module Gitlab return if permitted_call_count <= MAXIMUM_GITALY_CALLS - raise TooManyInvocationsError.new(call_site, actual_call_count, max_call_count, max_stacks) + # We've exceeded the limit, but we may still be in the presence of a non + # n+1 but still complex request with many distinct calls. If the maximum + # call count is 1 or less that's probably the case. + max_count = max_call_count + return if max_count <= 1 + + raise TooManyInvocationsError.new(call_site, actual_call_count, max_count, max_stacks) end def self.allow_n_plus_1_calls -- cgit v1.2.1 From a02881dfda8cc65cfa5f9eeeee5f2c24496cbb69 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 26 Sep 2017 20:07:27 +0200 Subject: RepositoryExists is always called with #gitaly_migration --- app/models/repository.rb | 18 +----------------- changelogs/unreleased/zj-repo-gitaly.yml | 5 +++++ lib/gitlab/git/repository.rb | 14 ++++++++++++-- 3 files changed, 18 insertions(+), 19 deletions(-) create mode 100644 changelogs/unreleased/zj-repo-gitaly.yml diff --git a/app/models/repository.rb b/app/models/repository.rb index b28fe79e19c..f0de2697dfc 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -489,13 +489,7 @@ class Repository def exists? return false unless full_path - Gitlab::GitalyClient.migrate(:repository_exists) do |enabled| - if enabled - raw_repository.exists? - else - refs_directory_exists? - end - end + raw_repository.exists? end cache_method :exists? @@ -1063,12 +1057,6 @@ class Repository blob.data end - def refs_directory_exists? - circuit_breaker.perform do - File.exist?(File.join(path_to_repo, 'refs')) - end - end - def cache # TODO: should we use UUIDs here? We could move repositories without clearing this cache @cache ||= RepositoryCache.new(full_path, @project.id) @@ -1120,10 +1108,6 @@ class Repository Gitlab::Git::Repository.new(project.repository_storage, disk_path + '.git', Gitlab::GlRepository.gl_repository(project, false)) end - def circuit_breaker - @circuit_breaker ||= Gitlab::Git::Storage::CircuitBreaker.for_storage(project.repository_storage) - end - def find_commits_by_message_by_shelling_out(query, ref, path, limit, offset) ref ||= root_ref diff --git a/changelogs/unreleased/zj-repo-gitaly.yml b/changelogs/unreleased/zj-repo-gitaly.yml new file mode 100644 index 00000000000..634f6ba1b8b --- /dev/null +++ b/changelogs/unreleased/zj-repo-gitaly.yml @@ -0,0 +1,5 @@ +--- +title: Gitaly RepositoryExists remains opt-in for all method calls +merge_request: +author: +type: fixed diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 616b075c087..8c1df650567 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -73,8 +73,6 @@ module Gitlab delegate :empty?, to: :rugged - delegate :exists?, to: :gitaly_repository_client - def ==(other) path == other.path end @@ -102,6 +100,18 @@ module Gitlab @circuit_breaker ||= Gitlab::Git::Storage::CircuitBreaker.for_storage(storage) end + def exists? + Gitlab::GitalyClient.migrate(:repository_exists) do |enabled| + if enabled + gitaly_repository_client.exists? + else + circuit_breaker.perform do + File.exist?(File.join(@path, 'refs')) + end + end + end + end + # Returns an Array of branch names # sorted by name ASC def branch_names -- cgit v1.2.1 From b4e9a79ff5c15ac07956f335a80dd468c8bdcf26 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Wed, 27 Sep 2017 09:44:12 +0100 Subject: Improves variable name --- app/views/shared/_auto_devops_callout.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/_auto_devops_callout.html.haml b/app/views/shared/_auto_devops_callout.html.haml index 88e1ed301d4..7c633175a06 100644 --- a/app/views/shared/_auto_devops_callout.html.haml +++ b/app/views/shared/_auto_devops_callout.html.haml @@ -10,6 +10,6 @@ %p= s_('AutoDevOps|Auto DevOps can be activated for this project. It will automatically build, test, and deploy your application based on a predefined CI/CD configuration.') %p - link = link_to(s_('AutoDevOps|Auto DevOps documentation'), help_page_path('topics/autodevops/index.md'), target: '_blank', rel: 'noopener noreferrer') - = s_('AutoDevOps|Learn more in the %{link}').html_safe % { link: link } + = s_('AutoDevOps|Learn more in the %{link_to_documentation}').html_safe % { link_to_documentation: link } = link_to s_('AutoDevOps|Enable in settings'), project_settings_ci_cd_path(@project, anchor: 'js-general-pipeline-settings'), class: 'btn btn-primary js-close-callout' -- cgit v1.2.1 From 26e73c2e8fa3d6cdd85ba82628981d3334445aeb Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 27 Sep 2017 11:45:16 +0200 Subject: Add some minor improvements to pipeline creation chain --- app/services/ci/create_pipeline_service.rb | 12 +++--------- lib/gitlab/ci/pipeline/chain/sequence.rb | 5 ++--- lib/gitlab/ci/pipeline/chain/validate/repository.rb | 2 -- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index df5b32d97ca..31a712ccc1b 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -15,7 +15,7 @@ module Ci ref: ref, sha: sha, before_sha: before_sha, - tag: tag?, + tag: tag_exists?, trigger_requests: Array(trigger_request), user: current_user, pipeline_schedule: schedule, @@ -88,20 +88,14 @@ module Ci params[:ref] end - def tag? - return @is_tag if defined?(@is_tag) - - @is_tag = project.repository.tag_exists?(ref) + def tag_exists? + project.repository.tag_exists?(ref) end def ref @ref ||= Gitlab::Git.ref_name(origin_ref) end - def valid_sha? - origin_sha && origin_sha != Gitlab::Git::BLANK_SHA - end - def pipeline_created_counter @pipeline_created_counter ||= Gitlab::Metrics .counter(:pipelines_created_total, "Counter of pipelines created") diff --git a/lib/gitlab/ci/pipeline/chain/sequence.rb b/lib/gitlab/ci/pipeline/chain/sequence.rb index c80d583939c..015f2988327 100644 --- a/lib/gitlab/ci/pipeline/chain/sequence.rb +++ b/lib/gitlab/ci/pipeline/chain/sequence.rb @@ -18,7 +18,7 @@ module Gitlab break if step.break? - @completed << true + @completed << step end @pipeline.tap do @@ -27,8 +27,7 @@ module Gitlab end def complete? - @completed.size == @sequence.size && - @completed.all? + @completed.size == @sequence.size end end end diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb index 9d328c9cedb..70a4cfdbdea 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/repository.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/repository.rb @@ -14,8 +14,6 @@ module Gitlab ## TODO, we check commit in the service, that is why # there is no repository access here. # - # Should we validate repository before building a pipeline? - # unless pipeline.sha return error('Commit not found') end -- cgit v1.2.1 From 8091cfc51de93da1826f4170717b9648afd621a8 Mon Sep 17 00:00:00 2001 From: Fabio Busatto Date: Wed, 27 Sep 2017 09:56:19 +0000 Subject: Add how to use reserved words in gitlab-ci.yml --- doc/ci/yaml/README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index aad81843299..38bd0450a09 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -1570,6 +1570,11 @@ Read more on [GitLab Pages user documentation](../../user/project/pages/index.md Each instance of GitLab CI has an embedded debug tool called Lint. You can find the link under `/ci/lint` of your gitlab instance. +## Using reserved keywords + +If you get validation error when using specific values (e.g., `true` or `false`), +try to quote them, or change them to a different form (e.g., `/bin/true`). + ## Skipping jobs If your commit message contains `[ci skip]` or `[skip ci]`, using any -- cgit v1.2.1 From 537dd5264fecfe281062be1371245bbec8018461 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Wed, 27 Sep 2017 11:13:17 +0100 Subject: Generates locale/gitlab.pot --- locale/gitlab.pot | 206 +++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 187 insertions(+), 19 deletions(-) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bc476a706cb..3a45b386ee1 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8,8 +8,8 @@ msgid "" msgstr "" "Project-Id-Version: gitlab 1.0.0\n" "Report-Msgid-Bugs-To: \n" -"POT-Creation-Date: 2017-09-21 14:20+0530\n" -"PO-Revision-Date: 2017-09-21 14:20+0530\n" +"POT-Creation-Date: 2017-09-27 11:10+0100\n" +"PO-Revision-Date: 2017-09-27 11:10+0100\n" "Last-Translator: FULL NAME \n" "Language-Team: LANGUAGE \n" "Language: \n" @@ -31,6 +31,9 @@ msgstr[1] "" msgid "%{commit_author_link} committed %{commit_timeago}" msgstr "" +msgid "%{number_commits_behind} commits behind %{default_branch}, %{number_commits_ahead} commits ahead" +msgstr "" + msgid "%{number_of_failures} of %{maximum_failures} failures. GitLab will allow access on the next attempt." msgstr "" @@ -131,25 +134,28 @@ msgstr "" msgid "Authentication Log" msgstr "" -msgid "Auto DevOps (Beta)" +msgid "Auto Review Apps and Auto Deploy need a domain name and the %{kubernetes} to work correctly." msgstr "" -msgid "Auto DevOps can be activated for this project. It will automatically build, test, and deploy your application based on a predefined CI/CD configuration." +msgid "Auto Review Apps and Auto Deploy need a domain name to work correctly." msgstr "" -msgid "Auto DevOps documentation" +msgid "Auto Review Apps and Auto Deploy need the %{kubernetes} to work correctly." msgstr "" -msgid "Auto Review Apps and Auto Deploy need a domain name and the %{kubernetes} to work correctly." +msgid "AutoDevOps|Auto DevOps (Beta)" msgstr "" -msgid "Auto Review Apps and Auto Deploy need a domain name to work correctly." +msgid "AutoDevOps|Auto DevOps can be activated for this project. It will automatically build, test, and deploy your application based on a predefined CI/CD configuration." msgstr "" -msgid "Auto Review Apps and Auto Deploy need the %{kubernetes} to work correctly." +msgid "AutoDevOps|Auto DevOps documentation" msgstr "" -msgid "AutoDevOps|Learn more in the" +msgid "AutoDevOps|Enable in settings" +msgstr "" + +msgid "AutoDevOps|Learn more in the %{link_to_documentation}" msgstr "" msgid "Board" @@ -172,12 +178,81 @@ msgstr "" msgid "Branches" msgstr "" +msgid "Branches|Cant find HEAD commit for this branch" +msgstr "" + +msgid "Branches|Compare" +msgstr "" + +msgid "Branches|Delete all branches that are merged into '%{default_branch}'" +msgstr "" + +msgid "Branches|Delete branch" +msgstr "" + +msgid "Branches|Delete merged branches" +msgstr "" + +msgid "Branches|Delete protected branch" +msgstr "" + +msgid "Branches|Delete protected branch '%{branch_name}'?" +msgstr "" + +msgid "Branches|Deleting the '%{branch_name}' branch cannot be undone. Are you sure?" +msgstr "" + +msgid "Branches|Deleting the merged branches cannot be undone. Are you sure?" +msgstr "" + +msgid "Branches|Filter by branch name" +msgstr "" + +msgid "Branches|Merged into %{default_branch}" +msgstr "" + +msgid "Branches|New branch" +msgstr "" + +msgid "Branches|No branches to show" +msgstr "" + +msgid "Branches|Once you confirm and press %{delete_protected_branch}, it cannot be undone or recovered." +msgstr "" + +msgid "Branches|Only a project master or owner can delete a protected branch" +msgstr "" + +msgid "Branches|Protected branches can be managed in %{project_settings_link}" +msgstr "" + +msgid "Branches|Sort by" +msgstr "" + +msgid "Branches|The default branch cannot be deleted" +msgstr "" + msgid "Branches|This branch hasn’t been merged into %{default_branch}." msgstr "" msgid "Branches|To avoid data loss, consider merging this branch before deleting it." msgstr "" +msgid "Branches|To confirm, type %{branch_name_confirmation}:" +msgstr "" + +msgid "Branches|You’re about to permanently delete the protected branch %{branch_name}." +msgstr "" + +msgid "Branches|merged" +msgstr "" + +msgid "Branches|project settings" +msgstr "" + +msgid "Branches|protected" +msgstr "" + msgid "Browse Directory" msgstr "" @@ -402,6 +477,12 @@ msgstr "" msgid "CycleAnalyticsStage|Test" msgstr "" +msgid "DashboardProjects|All" +msgstr "" + +msgid "DashboardProjects|Personal" +msgstr "" + msgid "Define a custom pattern with cron syntax" msgstr "" @@ -467,9 +548,6 @@ msgstr "" msgid "Emails" msgstr "" -msgid "Enable in settings" -msgstr "" - msgid "EventFilterBy|Filter by all" msgstr "" @@ -568,7 +646,7 @@ msgstr "" msgid "GroupSettings|This setting is applied on %{ancestor_group}. You can override the setting or %{remove_ancestor_share_with_group_lock}." msgstr "" -msgid "GroupSettings|This setting will be applied to all subgroups unless overridden by a group owner." +msgid "GroupSettings|This setting will be applied to all subgroups unless overridden by a group owner. Groups that already have access to the project will continue to have access unless removed manually." msgstr "" msgid "GroupSettings|cannot be disabled when the parent group \"Share with group lock\" is enabled, except by the owner of the parent group" @@ -595,9 +673,6 @@ msgstr "" msgid "HealthCheck|Unhealthy" msgstr "" -msgid "Home" -msgstr "" - msgid "Housekeeping successfully started" msgstr "" @@ -680,6 +755,9 @@ msgstr "" msgid "Merge events" msgstr "" +msgid "Merge request" +msgstr "" + msgid "Messages" msgstr "" @@ -955,9 +1033,6 @@ msgstr "" msgid "Project export started. A download link will be sent by email." msgstr "" -msgid "Project home" -msgstr "" - msgid "ProjectActivityRSS|Subscribe" msgstr "" @@ -1128,6 +1203,93 @@ msgstr[1] "" msgid "Snippets" msgstr "" +msgid "SortOptions|Access level, ascending" +msgstr "" + +msgid "SortOptions|Access level, descending" +msgstr "" + +msgid "SortOptions|Created date" +msgstr "" + +msgid "SortOptions|Due date" +msgstr "" + +msgid "SortOptions|Due later" +msgstr "" + +msgid "SortOptions|Due soon" +msgstr "" + +msgid "SortOptions|Label priority" +msgstr "" + +msgid "SortOptions|Largest group" +msgstr "" + +msgid "SortOptions|Largest repository" +msgstr "" + +msgid "SortOptions|Last created" +msgstr "" + +msgid "SortOptions|Last joined" +msgstr "" + +msgid "SortOptions|Last updated" +msgstr "" + +msgid "SortOptions|Least popular" +msgstr "" + +msgid "SortOptions|Milestone" +msgstr "" + +msgid "SortOptions|Milestone due later" +msgstr "" + +msgid "SortOptions|Milestone due soon" +msgstr "" + +msgid "SortOptions|Most popular" +msgstr "" + +msgid "SortOptions|Name" +msgstr "" + +msgid "SortOptions|Name, ascending" +msgstr "" + +msgid "SortOptions|Name, descending" +msgstr "" + +msgid "SortOptions|Oldest created" +msgstr "" + +msgid "SortOptions|Oldest joined" +msgstr "" + +msgid "SortOptions|Oldest sign in" +msgstr "" + +msgid "SortOptions|Oldest updated" +msgstr "" + +msgid "SortOptions|Popularity" +msgstr "" + +msgid "SortOptions|Priority" +msgstr "" + +msgid "SortOptions|Recent sign in" +msgstr "" + +msgid "SortOptions|Start later" +msgstr "" + +msgid "SortOptions|Start soon" +msgstr "" + msgid "Source code" msgstr "" @@ -1398,9 +1560,15 @@ msgstr "" msgid "Use your global notification setting" msgstr "" +msgid "View file @ " +msgstr "" + msgid "View open merge request" msgstr "" +msgid "View replaced file @ " +msgstr "" + msgid "VisibilityLevel|Internal" msgstr "" -- cgit v1.2.1 From 147cbb95f5ffc0d70ab8f0fc573f0847fc90b3f3 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Wed, 27 Sep 2017 12:13:50 +0100 Subject: Removes section about big MRs --- doc/development/fe_guide/index.md | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md index d84801f91d4..031b12a8e91 100644 --- a/doc/development/fe_guide/index.md +++ b/doc/development/fe_guide/index.md @@ -29,34 +29,6 @@ For our currently-supported browsers, see our [requirements][requirements]. ## Development Process -When you are assigned an issue please follow the next steps: - -### Divide a big feature into small Merge Requests -1. Big Merge Request are painful to review. In order to make this process easier we -must break a big feature into smaller ones and create a Merge Request for each step. -1. First step is to create a branch from `master`, let's call it `new-feature`. This branch -will be the recipient of all the smaller Merge Requests. Only this one will be merged to master. -1. Don't do any work on this one, let's keep it synced with master. -1. Create a new branch from `new-feature`, let's call it `new-feature-step-1`. We advise you -to clearly identify which step the branch represents. -1. Do the first part of the modifications in this branch. The target branch of this Merge Request -should be `new-feature`. -1. Once `new-feature-step-1` gets merged into `new-feature` we can continue our work. Create a new -branch from `new-feature`, let's call it `new-feature-step-2` and repeat the process done before. - -```shell -master -└─ new-feature - ├─ new-feature-step-1 - ├─ new-feature-step-2 - └─ new-feature-step-3 -``` - -**Tips** -- Make sure `new-feature` branch is always synced with `master`: merge master frequently. -- Do the same for the feature branch you have opened. This can be accomplished by merging `master` into `new-feature` and `new-feature` into `new-feature-step-*` -- Avoid rewriting history. - ### Share your work early 1. Before writing code guarantee your vision of the architecture is aligned with GitLab's architecture. -- cgit v1.2.1 From 73f27db26407862c32d5776c60eb0c4c55261405 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 26 Sep 2017 12:50:54 +0200 Subject: Update CI parallelization description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- doc/development/testing.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/doc/development/testing.md b/doc/development/testing.md index 83269303005..c9f14b5fb35 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -493,24 +493,24 @@ Here are some things to keep in mind regarding test performance: Our current CI parallelization setup is as follows: -1. The `knapsack` job in the prepare stage that is supposed to ensure we have a - `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file: +1. The `retrieve-tests-metadata` job in the `prepare` stage ensures that we have + a `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file: - The `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file is fetched from S3, if it's not here we initialize the file with `{}`. -1. Each `rspec x y` job are run with `knapsack rspec` and should have an evenly - distributed share of tests: +1. Each `rspec-pg x y`/`rspec-mysql x y` job is run with `knapsack rspec` and + should have an evenly distributed share of tests: - It works because the jobs have access to the `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` since the "artifacts from all previous stages are passed by default". [^1] - - the jobs set their own report path to + - The jobs set their own report path to `KNAPSACK_REPORT_PATH=knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json`. - - if knapsack is doing its job, test files that are run should be listed under + - If knapsack is doing its job, test files that are run should be listed under `Report specs`, not under `Leftover specs`. -1. The `update-knapsack` job takes all the +1. The `update-tests-metadata` job takes all the `knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json` - files from the `rspec x y` jobs and merge them all together into a single - `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file that is then - uploaded to S3. + files from the `rspec-pg x y`/`rspec-mysql x y`jobs and merge them all together + into a single `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file that + is then uploaded to S3. After that, the next pipeline will use the up-to-date `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file. The same strategy -- cgit v1.2.1 From 0612ee64bc4b0baebf8f47c1eda77823403d7473 Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Wed, 27 Sep 2017 13:51:23 +0200 Subject: Fix linting errors in locale/index.js --- .eslintignore | 2 +- app/assets/javascripts/locale/index.js | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.eslintignore b/.eslintignore index d6ce39636bd..1623b996213 100644 --- a/.eslintignore +++ b/.eslintignore @@ -8,4 +8,4 @@ karma.config.js webpack.config.js svg.config.js -/app/assets/javascripts/locale/**/*.js +/app/assets/javascripts/locale/**/app.js diff --git a/app/assets/javascripts/locale/index.js b/app/assets/javascripts/locale/index.js index 7ba676d6d20..6a5084efeb8 100644 --- a/app/assets/javascripts/locale/index.js +++ b/app/assets/javascripts/locale/index.js @@ -16,9 +16,8 @@ const locales = allLocales.reduce((d, obj) => { return data; }, {}); -let lang = document.querySelector('html').getAttribute('lang') || 'en'; -lang = lang.replace(/-/g, '_'); - +const langAttribute = document.querySelector('html').getAttribute('lang'); +const lang = (langAttribute || 'en').replace(/-/g, '_'); const locale = new Jed(locales[lang]); /** -- cgit v1.2.1 From 3d611b9cdb29a5b480adea630b26e0c6c05434f7 Mon Sep 17 00:00:00 2001 From: Jarka Kadlecova Date: Wed, 27 Sep 2017 14:33:30 +0200 Subject: Update CHANGELOG.md for 10.0.2 [ci skip] --- CHANGELOG.md | 7 +++++++ changelogs/unreleased/37912-fix-dash-in-note-access-role.yml | 5 ----- .../38280-undefined-run_command-when-running-rake-gitlab-check.yml | 5 ----- changelogs/unreleased/fix-locked-shared-runners-problem.yml | 5 ----- changelogs/unreleased/rs-allow-name-on-anchors.yml | 5 ----- 5 files changed, 7 insertions(+), 20 deletions(-) delete mode 100644 changelogs/unreleased/37912-fix-dash-in-note-access-role.yml delete mode 100644 changelogs/unreleased/38280-undefined-run_command-when-running-rake-gitlab-check.yml delete mode 100644 changelogs/unreleased/fix-locked-shared-runners-problem.yml delete mode 100644 changelogs/unreleased/rs-allow-name-on-anchors.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c671f8d53a..d08e42b3b65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 10.0.2 (2017-09-27) + +- [FIXED] Notes will not show an empty bubble when the author isn't a member. !14450 +- [FIXED] Some checks in `rake gitlab:check` were failling with 'undefined method `run_command`'. !14469 +- [FIXED] Make locked setting of Runner to not affect jobs scheduling. !14483 +- [FIXED] Re-allow `name` attribute on user-provided anchor HTML. + ## 10.0.1 (2017-09-23) - [FIXED] Fix duplicate key errors in PostDeployMigrateUserExternalMailData migration. diff --git a/changelogs/unreleased/37912-fix-dash-in-note-access-role.yml b/changelogs/unreleased/37912-fix-dash-in-note-access-role.yml deleted file mode 100644 index f9f4120479c..00000000000 --- a/changelogs/unreleased/37912-fix-dash-in-note-access-role.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Notes will not show an empty bubble when the author isn't a member. -merge_request: 14450 -author: -type: fixed diff --git a/changelogs/unreleased/38280-undefined-run_command-when-running-rake-gitlab-check.yml b/changelogs/unreleased/38280-undefined-run_command-when-running-rake-gitlab-check.yml deleted file mode 100644 index 7d3fb7d43cc..00000000000 --- a/changelogs/unreleased/38280-undefined-run_command-when-running-rake-gitlab-check.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Some checks in `rake gitlab:check` were failling with 'undefined method `run_command`' -merge_request: 14469 -author: -type: fixed diff --git a/changelogs/unreleased/fix-locked-shared-runners-problem.yml b/changelogs/unreleased/fix-locked-shared-runners-problem.yml deleted file mode 100644 index 3e3cccf79eb..00000000000 --- a/changelogs/unreleased/fix-locked-shared-runners-problem.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Make locked setting of Runner to not affect jobs scheduling -merge_request: 14483 -author: -type: fixed diff --git a/changelogs/unreleased/rs-allow-name-on-anchors.yml b/changelogs/unreleased/rs-allow-name-on-anchors.yml deleted file mode 100644 index 59e95ed8a0e..00000000000 --- a/changelogs/unreleased/rs-allow-name-on-anchors.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Re-allow `name` attribute on user-provided anchor HTML -merge_request: -author: -type: fixed -- cgit v1.2.1 From fe24c0a875fa1d6f561a5b228cf71d0c9da671af Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 27 Sep 2017 15:38:13 +0100 Subject: Fixes commit comments in side-by-side diff view This was caused by the `notes` global class not existing when the `file_comment_button` code is run. The notes class was used to check if the diff is currently in parallel view or not. To get around this I've added a check into the `file_comment_button` JS to check if the view is currently parallel or not. Closes #38117 --- app/assets/javascripts/files_comment_button.js | 6 ++-- .../unreleased/commit-side-by-side-comment.yml | 5 +++ spec/features/projects/commit/diff_notes_spec.rb | 36 ++++++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/commit-side-by-side-comment.yml create mode 100644 spec/features/projects/commit/diff_notes_spec.rb diff --git a/app/assets/javascripts/files_comment_button.js b/app/assets/javascripts/files_comment_button.js index d02e4cd5876..a00d29a845a 100644 --- a/app/assets/javascripts/files_comment_button.js +++ b/app/assets/javascripts/files_comment_button.js @@ -7,6 +7,8 @@ * causes reflows, visit https://gist.github.com/paulirish/5d52fb081b3570c81e3a */ +import Cookies from 'js-cookie'; + const LINE_NUMBER_CLASS = 'diff-line-num'; const UNFOLDABLE_LINE_CLASS = 'js-unfold'; const NO_COMMENT_CLASS = 'no-comment-btn'; @@ -27,9 +29,7 @@ export default { this.userCanCreateNote = $diffFile.closest(DIFF_CONTAINER_SELECTOR).data('can-create-note') === ''; } - if (typeof notes !== 'undefined' && !this.isParallelView) { - this.isParallelView = notes.isParallelView && notes.isParallelView(); - } + this.isParallelView = Cookies.get('diff_view') === 'parallel'; if (this.userCanCreateNote) { $diffFile.on('mouseover', LINE_COLUMN_CLASSES, e => this.showButton(this.isParallelView, e)) diff --git a/changelogs/unreleased/commit-side-by-side-comment.yml b/changelogs/unreleased/commit-side-by-side-comment.yml new file mode 100644 index 00000000000..f9bea285a77 --- /dev/null +++ b/changelogs/unreleased/commit-side-by-side-comment.yml @@ -0,0 +1,5 @@ +--- +title: Fixed commenting on side-by-side commit diff +merge_request: +author: +type: fixed diff --git a/spec/features/projects/commit/diff_notes_spec.rb b/spec/features/projects/commit/diff_notes_spec.rb new file mode 100644 index 00000000000..f0fe4e00acc --- /dev/null +++ b/spec/features/projects/commit/diff_notes_spec.rb @@ -0,0 +1,36 @@ +require 'spec_helper' + +feature 'Commit diff', :js do + include RepoHelpers + + let(:user) { create(:user) } + let(:project) { create(:project, :public, :repository) } + + before do + project.add_master(user) + sign_in user + end + + %w(inline parallel).each do |view| + context "#{view} view" do + before do + visit project_commit_path(project, sample_commit.id, view: view) + end + + it "adds comment to diff" do + diff_line_num = first('.diff-line-num.new') + + diff_line_num.trigger('mouseover') + diff_line_num.find('.js-add-diff-note-button').trigger('click') + + page.within(first('.diff-viewer')) do + find('.js-note-text').set 'test comment' + + click_button 'Comment' + + expect(page).to have_content('test comment') + end + end + end + end +end -- cgit v1.2.1 From d2f96827b098bebedc41f0d4be86e44fd2ef3a56 Mon Sep 17 00:00:00 2001 From: Guilherme Vieira Date: Fri, 22 Sep 2017 22:26:00 -0300 Subject: Check orientation of profile image --- app/assets/javascripts/profile/gl_crop.js | 3 ++- changelogs/unreleased/rotated_profile_image.yml | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/rotated_profile_image.yml diff --git a/app/assets/javascripts/profile/gl_crop.js b/app/assets/javascripts/profile/gl_crop.js index 291ae24aa68..4bdda611cfc 100644 --- a/app/assets/javascripts/profile/gl_crop.js +++ b/app/assets/javascripts/profile/gl_crop.js @@ -73,7 +73,8 @@ import _ from 'underscore'; aspectRatio: 1, modal: true, scalable: false, - rotatable: false, + rotatable: true, + checkOrientation: true, zoomable: true, dragMode: 'move', guides: false, diff --git a/changelogs/unreleased/rotated_profile_image.yml b/changelogs/unreleased/rotated_profile_image.yml new file mode 100644 index 00000000000..1e221e47379 --- /dev/null +++ b/changelogs/unreleased/rotated_profile_image.yml @@ -0,0 +1,5 @@ +--- +title: Fix profile image orientation based on EXIF data gvieira37 +merge_request: 14461 +author: gvieira37 +type: fixed -- cgit v1.2.1 From 0aef7d27e55e0a0e99b121ca84b41f4c86135725 Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Wed, 27 Sep 2017 14:59:51 +0000 Subject: Added some Gitaly docs to `docs/development` --- doc/development/README.md | 1 + doc/development/gitaly.md | 54 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 doc/development/gitaly.md diff --git a/doc/development/README.md b/doc/development/README.md index 3096d9f25f0..1448a4c0414 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -44,6 +44,7 @@ - [Building a package for testing purposes](build_test_package.md) - [Manage feature flags](feature_flags.md) - [View sent emails or preview mailers](emails.md) +- [Working with Gitaly](gitaly.md) ## Databases diff --git a/doc/development/gitaly.md b/doc/development/gitaly.md new file mode 100644 index 00000000000..f0be3a6b141 --- /dev/null +++ b/doc/development/gitaly.md @@ -0,0 +1,54 @@ +# GitLab Developers Guide to Working with Gitaly + +[Gitaly](https://gitlab.com/gitlab-org/gitaly) is a high-level Git RPC service used by GitLab CE/EE, +Workhorse and GitLab-Shell. All Rugged operations in GitLab CE/EE are currently being phased out to +be replaced by Gitaly API calls. + +Visit the [Gitaly Migration Board](https://gitlab.com/gitlab-org/gitaly/boards/331341) for current +status of the migration. + +## Feature Flags + +Gitaly makes heavy use of [feature flags](feature_flags.md). + +Each Rugged-to-Gitaly migration goes through a [series of phases](https://gitlab.com/gitlab-org/gitaly/blob/master/doc/MIGRATION_PROCESS.md): +* **Opt-In**: by default the Rugged implementation is used. + * Production instances can choose to enable the Gitaly endpoint by enabling the feature flag. + * For testing purposes, you may wish to enable all feature flags by default. This can be done by exporting the following + environment variable: `GITALY_FEATURE_DEFAULT_ON=1`. + * On developer instances (ie, when `Rails.env.development?` is true), the Gitaly endpoint + is enabled by default, but can be _disabled_ using feature flags. +* **Opt-Out**: by default, the Gitaly endpoint is used, but the feature can be explicitly disabled using the feature flag. +* **Madatory**: The migration is complete and cannot be disabled. The old codepath is removed. + +### Enabling and Disabling Feature + +In the Rails console, type: + +```ruby +Feature.enable(:gitaly_feature_name) +Feature.disable(:gitaly_feature_name) +``` + +Where `gitaly_feature_name` is the name of the Gitaly feature. This can be determined by finding the appropriate +`gitaly_migrate` code block, for example: + +```ruby +gitaly_migrate(:tag_names) do +... +end +``` + +Since Gitaly features are always prefixed with `gitaly_`, the name of the feature flag in this case would be `gitaly_tag_names`. + +## Gitaly-Related Test Failures + +If your test-suite is failing with Gitaly issues, as a first step, try running: + +```shell +rm -rf tmp/tests/gitaly +``` + +--- + +[Return to Development documentation](README.md) -- cgit v1.2.1 From 864790b8bb2146ea500323feacce91243ebd0b06 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Wed, 27 Sep 2017 17:29:10 +0200 Subject: Add EEP to headings --- doc/user/project/integrations/kubernetes.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/doc/user/project/integrations/kubernetes.md b/doc/user/project/integrations/kubernetes.md index 7f3a000127f..e9738b683f9 100644 --- a/doc/user/project/integrations/kubernetes.md +++ b/doc/user/project/integrations/kubernetes.md @@ -10,10 +10,6 @@ Kubernetes API (such as OpenShift). Each project can be configured to connect to a different Kubernetes cluster, see the [configuration](#configuration) section. -If you have a single cluster that you want to use for all your projects, -you can pre-fill the settings page with a default template. To configure the -template, see the [Services Templates](services_templates.md) document. - ## Configuration Navigate to the [Integrations page](project_services.md#accessing-the-project-services) @@ -51,6 +47,11 @@ The Kubernetes service takes the following parameters: [Kubernetes dashboard](https://kubernetes.io/docs/tasks/access-application-cluster/web-ui-dashboard/#config) (under **Config > Secrets**). +TIP: **Tip:** +If you have a single cluster that you want to use for all your projects, +you can pre-fill the settings page with a default template. To configure the +template, see [Services Templates](services_templates.md). + ## Deployment variables The Kubernetes service exposes the following @@ -73,7 +74,7 @@ GitLab CI/CD build environment: Here's what you can do with GitLab if you enable the Kubernetes integration. -### Deploy Boards +### Deploy Boards (EEP) > Available in [GitLab Enterprise Edition Premium][ee]. @@ -85,7 +86,7 @@ workflow they already use without any need to access Kubernetes. [> Read more about Deploy Boards](https://docs.gitlab.com/ee/user/project/deploy_boards.html) -### Canary Deployments +### Canary Deployments (EEP) > Available in [GitLab Enterprise Edition Premium][ee]. @@ -100,7 +101,7 @@ the need to leave GitLab. Automatically detect and monitor Kubernetes metrics. Automatic monitoring of [NGINX ingress](./prometheus_library/nginx.md) is also supported. -[> Read more about Kubernetes monitoring](./prometheus_library/kubernetes.md) +[> Read more about Kubernetes monitoring](prometheus_library/kubernetes.md) ### Auto DevOps -- cgit v1.2.1 From e8616997d3ae8cf7eb8d39d0c67cd0849f4d32f5 Mon Sep 17 00:00:00 2001 From: Dimitrie Hoekstra Date: Wed, 27 Sep 2017 16:46:17 +0000 Subject: Breadcrumbs receives padding when double lined --- app/assets/stylesheets/framework/new-nav.scss | 5 ++++- changelogs/unreleased/breadcrumbs-line-height-padding.yml | 5 +++++ 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/breadcrumbs-line-height-padding.yml diff --git a/app/assets/stylesheets/framework/new-nav.scss b/app/assets/stylesheets/framework/new-nav.scss index d4b3fb238d5..81022d4af2a 100644 --- a/app/assets/stylesheets/framework/new-nav.scss +++ b/app/assets/stylesheets/framework/new-nav.scss @@ -306,6 +306,8 @@ header.navbar-gitlab-new { display: flex; width: 100%; position: relative; + padding-top: $gl-padding / 2; + padding-bottom: $gl-padding / 2; align-items: center; border-bottom: 1px solid $border-color; } @@ -346,6 +348,7 @@ header.navbar-gitlab-new { display: flex; align-items: center; position: relative; + padding: 2px 0; &:not(:last-child) { margin-right: 20px; @@ -381,7 +384,7 @@ header.navbar-gitlab-new { margin: 0; font-size: 12px; font-weight: 600; - line-height: 1; + line-height: 16px; a { color: $gl-text-color; diff --git a/changelogs/unreleased/breadcrumbs-line-height-padding.yml b/changelogs/unreleased/breadcrumbs-line-height-padding.yml new file mode 100644 index 00000000000..3ac56c8b593 --- /dev/null +++ b/changelogs/unreleased/breadcrumbs-line-height-padding.yml @@ -0,0 +1,5 @@ +--- +title: breadcrumbs receives padding when double lined +merge_request: +author: +type: changed -- cgit v1.2.1 From 121057c52b65b1ac53bebe62312485918bdd863a Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Wed, 27 Sep 2017 19:28:36 +0100 Subject: Rolling back change to n+1 detection --- lib/gitlab/gitaly_client.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index 3f10951f49e..955d2307f88 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -165,13 +165,7 @@ module Gitlab return if permitted_call_count <= MAXIMUM_GITALY_CALLS - # We've exceeded the limit, but we may still be in the presence of a non - # n+1 but still complex request with many distinct calls. If the maximum - # call count is 1 or less that's probably the case. - max_count = max_call_count - return if max_count <= 1 - - raise TooManyInvocationsError.new(call_site, actual_call_count, max_count, max_stacks) + raise TooManyInvocationsError.new(call_site, actual_call_count, max_call_count, max_stacks) end def self.allow_n_plus_1_calls -- cgit v1.2.1 From e38dc10c09ac6e6d7752d2bb9eeb0c3feab9765a Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Wed, 27 Sep 2017 17:38:23 -0300 Subject: Clean merge_jid whenever necessary on the merge process MergeRequest#merge_jid should be cleaned up whenever we hit a known error on MergeService#execute. This way we can keep track if the MR is really "ongoing" or "stuck" --- app/services/merge_requests/merge_service.rb | 19 +++++++++++------- ...-improve-merge-jid-cleanup-on-merge-process.yml | 5 +++++ spec/services/merge_requests/merge_service_spec.rb | 23 +++++++++++++++++++--- 3 files changed, 37 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/38476-improve-merge-jid-cleanup-on-merge-process.yml diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 07cbd8f92a9..bf26859dd6d 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -14,13 +14,13 @@ module MergeRequests @merge_request = merge_request unless @merge_request.mergeable? - return log_merge_error('Merge request is not mergeable', save_message_on_model: true) + return handle_merge_error(log_message: 'Merge request is not mergeable', save_message_on_model: true) end @source = find_merge_source unless @source - return log_merge_error('No source for merge', save_message_on_model: true) + return handle_merge_error(log_message: 'No source for merge', save_message_on_model: true) end merge_request.in_locked_state do @@ -31,8 +31,7 @@ module MergeRequests end end rescue MergeError => e - clean_merge_jid - log_merge_error(e.message, save_message_on_model: true) + handle_merge_error(log_message: e.message, save_message_on_model: true) end private @@ -74,10 +73,16 @@ module MergeRequests @merge_request.force_remove_source_branch? ? @merge_request.author : current_user end - def log_merge_error(message, save_message_on_model: false) - Rails.logger.error("MergeService ERROR: #{merge_request_info} - #{message}") + # Logs merge error message and cleans `MergeRequest#merge_jid`. + # + def handle_merge_error(log_message:, save_message_on_model: false) + Rails.logger.error("MergeService ERROR: #{merge_request_info} - #{log_message}") - @merge_request.update(merge_error: message) if save_message_on_model + if save_message_on_model + @merge_request.update(merge_error: log_message, merge_jid: nil) + else + clean_merge_jid + end end def merge_request_info diff --git a/changelogs/unreleased/38476-improve-merge-jid-cleanup-on-merge-process.yml b/changelogs/unreleased/38476-improve-merge-jid-cleanup-on-merge-process.yml new file mode 100644 index 00000000000..43dec51029b --- /dev/null +++ b/changelogs/unreleased/38476-improve-merge-jid-cleanup-on-merge-process.yml @@ -0,0 +1,5 @@ +--- +title: Adjust MRs being stuck on "process of being merged" for more than 2 hours +merge_request: +author: +type: fixed diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index b60136064b7..80213d093f1 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -13,20 +13,21 @@ describe MergeRequests::MergeService do describe '#execute' do context 'MergeRequest#merge_jid' do + let(:service) do + described_class.new(project, user, commit_message: 'Awesome message') + end + before do merge_request.update_column(:merge_jid, 'hash-123') end it 'is cleaned when no error is raised' do - service = described_class.new(project, user, commit_message: 'Awesome message') - service.execute(merge_request) expect(merge_request.reload.merge_jid).to be_nil end it 'is cleaned when expected error is raised' do - service = described_class.new(project, user, commit_message: 'Awesome message') allow(service).to receive(:commit).and_raise(described_class::MergeError) service.execute(merge_request) @@ -34,6 +35,22 @@ describe MergeRequests::MergeService do expect(merge_request.reload.merge_jid).to be_nil end + it 'is cleaned when merge request is not mergeable' do + allow(merge_request).to receive(:mergeable?).and_return(false) + + service.execute(merge_request) + + expect(merge_request.reload.merge_jid).to be_nil + end + + it 'is cleaned when no source is found' do + allow(merge_request).to receive(:diff_head_sha).and_return(nil) + + service.execute(merge_request) + + expect(merge_request.reload.merge_jid).to be_nil + end + it 'is not cleaned when unexpected error is raised' do service = described_class.new(project, user, commit_message: 'Awesome message') allow(service).to receive(:commit).and_raise(StandardError) -- cgit v1.2.1 From ce6fb619e570cbe1f8315c48181cda5f003b7e15 Mon Sep 17 00:00:00 2001 From: Mark Fletcher Date: Thu, 28 Sep 2017 11:33:37 +0700 Subject: Change recommended MySQL version to 5.6 --- changelogs/unreleased/docs-38152-bump-recommended-mysql-version.yml | 5 +++++ doc/install/database_mysql.md | 6 ++++-- 2 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/docs-38152-bump-recommended-mysql-version.yml diff --git a/changelogs/unreleased/docs-38152-bump-recommended-mysql-version.yml b/changelogs/unreleased/docs-38152-bump-recommended-mysql-version.yml new file mode 100644 index 00000000000..eea679d0814 --- /dev/null +++ b/changelogs/unreleased/docs-38152-bump-recommended-mysql-version.yml @@ -0,0 +1,5 @@ +--- +title: Change recommended MySQL version to 5.6 +merge_request: +author: +type: other diff --git a/doc/install/database_mysql.md b/doc/install/database_mysql.md index 5c128f54a76..f9ba1508705 100644 --- a/doc/install/database_mysql.md +++ b/doc/install/database_mysql.md @@ -1,11 +1,12 @@ # Database MySQL >**Note:** -We do not recommend using MySQL due to various issues. For example, case +- 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). +- We recommend using MySQL version 5.6 or later. Please see the following [issue][ce-38152]. ## Initial database setup @@ -13,7 +14,7 @@ and [problems](https://bugs.mysql.com/bug.php?id=65830) that # Install the database packages sudo apt-get install -y mysql-server mysql-client libmysqlclient-dev -# Ensure you have MySQL version 5.5.14 or later +# Ensure you have MySQL version 5.6 or later mysql --version # Pick a MySQL root password (can be anything), type it and press enter @@ -293,3 +294,4 @@ Details can be found in the [PostgreSQL][postgres-text-type] and [postgres-text-type]: http://www.postgresql.org/docs/9.2/static/datatype-character.html [mysql-text-types]: http://dev.mysql.com/doc/refman/5.7/en/string-type-overview.html +[ce-38152]: https://gitlab.com/gitlab-org/gitlab-ce/issues/38152 -- cgit v1.2.1 From 9621dd0c9d31508bdac2e2e226537302b560ef10 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 20 Sep 2017 11:00:06 +0200 Subject: refactor services to match EE signature --- app/controllers/admin/users_controller.rb | 6 +++--- app/controllers/profiles/avatars_controller.rb | 2 +- app/controllers/profiles/emails_controller.rb | 4 ++-- app/controllers/profiles/notifications_controller.rb | 2 +- app/controllers/profiles/passwords_controller.rb | 6 +++--- app/controllers/profiles/preferences_controller.rb | 2 +- app/controllers/profiles/two_factor_auths_controller.rb | 6 +++--- app/controllers/profiles_controller.rb | 10 +++++----- app/controllers/sessions_controller.rb | 2 +- app/models/user.rb | 10 +++++----- app/services/emails/destroy_service.rb | 4 ++-- app/services/users/update_service.rb | 12 +++++++----- lib/api/internal.rb | 2 +- lib/api/notification_settings.rb | 2 +- lib/api/users.rb | 10 +++++----- lib/gitlab/ldap/access.rb | 2 +- lib/gitlab/o_auth/user.rb | 2 +- 17 files changed, 43 insertions(+), 41 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index cbcef70e957..8b6119548ee 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -128,7 +128,7 @@ class Admin::UsersController < Admin::ApplicationController end respond_to do |format| - result = Users::UpdateService.new(user, user_params_with_pass).execute do |user| + result = Users::UpdateService.new(current_user, user, user_params_with_pass).execute do |user| user.skip_reconfirmation! end @@ -155,7 +155,7 @@ class Admin::UsersController < Admin::ApplicationController def remove_email email = user.emails.find(params[:email_id]) - success = Emails::DestroyService.new(user, email: email.email).execute + success = Emails::DestroyService.new(current_user, user, email: email.email).execute respond_to do |format| if success @@ -219,7 +219,7 @@ class Admin::UsersController < Admin::ApplicationController end def update_user(&block) - result = Users::UpdateService.new(user).execute(&block) + result = Users::UpdateService.new(current_user, user).execute(&block) result[:status] == :success end diff --git a/app/controllers/profiles/avatars_controller.rb b/app/controllers/profiles/avatars_controller.rb index 408650aac54..5a94dc88455 100644 --- a/app/controllers/profiles/avatars_controller.rb +++ b/app/controllers/profiles/avatars_controller.rb @@ -2,7 +2,7 @@ class Profiles::AvatarsController < Profiles::ApplicationController def destroy @user = current_user - Users::UpdateService.new(@user).execute { |user| user.remove_avatar! } + Users::UpdateService.new(current_user, @user).execute { |user| user.remove_avatar! } redirect_to profile_path, status: 302 end diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb index ddb67d1c4d1..8328d230276 100644 --- a/app/controllers/profiles/emails_controller.rb +++ b/app/controllers/profiles/emails_controller.rb @@ -5,7 +5,7 @@ class Profiles::EmailsController < Profiles::ApplicationController end def create - @email = Emails::CreateService.new(current_user, email_params).execute + @email = Emails::CreateService.new(current_user, current_user, email_params).execute if @email.errors.blank? NotificationService.new.new_email(@email) @@ -19,7 +19,7 @@ class Profiles::EmailsController < Profiles::ApplicationController def destroy @email = current_user.emails.find(params[:id]) - Emails::DestroyService.new(current_user, email: @email.email).execute + Emails::DestroyService.new(current_user, current_user, email: @email.email).execute respond_to do |format| format.html { redirect_to profile_emails_url, status: 302 } diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb index 960b7512602..45d7bca27bb 100644 --- a/app/controllers/profiles/notifications_controller.rb +++ b/app/controllers/profiles/notifications_controller.rb @@ -7,7 +7,7 @@ class Profiles::NotificationsController < Profiles::ApplicationController end def update - result = Users::UpdateService.new(current_user, user_params).execute + result = Users::UpdateService.new(current_user, current_user, user_params).execute if result[:status] == :success flash[:notice] = "Notification settings saved" diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb index 7beb52dd8e8..08b438de9e2 100644 --- a/app/controllers/profiles/passwords_controller.rb +++ b/app/controllers/profiles/passwords_controller.rb @@ -21,10 +21,10 @@ class Profiles::PasswordsController < Profiles::ApplicationController password_automatically_set: false } - result = Users::UpdateService.new(@user, password_attributes).execute + result = Users::UpdateService.new(current_user, @user, password_attributes).execute if result[:status] == :success - Users::UpdateService.new(@user, password_expires_at: nil).execute + Users::UpdateService.new(current_user, @user, password_expires_at: nil).execute redirect_to root_path, notice: 'Password successfully changed' else @@ -46,7 +46,7 @@ class Profiles::PasswordsController < Profiles::ApplicationController return end - result = Users::UpdateService.new(@user, password_attributes).execute + result = Users::UpdateService.new(current_user, @user, password_attributes).execute if result[:status] == :success flash[:notice] = "Password was successfully updated. Please login with it" diff --git a/app/controllers/profiles/preferences_controller.rb b/app/controllers/profiles/preferences_controller.rb index cce2a847b53..a13b9a616cb 100644 --- a/app/controllers/profiles/preferences_controller.rb +++ b/app/controllers/profiles/preferences_controller.rb @@ -6,7 +6,7 @@ class Profiles::PreferencesController < Profiles::ApplicationController def update begin - result = Users::UpdateService.new(user, preferences_params).execute + result = Users::UpdateService.new(current_user, user, preferences_params).execute if result[:status] == :success flash[:notice] = 'Preferences saved.' diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index 1a4f77639e7..b590846257b 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -10,7 +10,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController current_user.otp_grace_period_started_at = Time.current end - Users::UpdateService.new(current_user).execute! + Users::UpdateService.new(current_user, current_user).execute! if two_factor_authentication_required? && !current_user.two_factor_enabled? two_factor_authentication_reason( @@ -41,7 +41,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController def create if current_user.validate_and_consume_otp!(params[:pin_code]) - Users::UpdateService.new(current_user, otp_required_for_login: true).execute! do |user| + Users::UpdateService.new(current_user, current_user, otp_required_for_login: true).execute! do |user| @codes = user.generate_otp_backup_codes! end @@ -70,7 +70,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController end def codes - Users::UpdateService.new(current_user).execute! do |user| + Users::UpdateService.new(current_user, current_user).execute! do |user| @codes = user.generate_otp_backup_codes! end end diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index d83824fef06..9e85997cf6d 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -10,7 +10,7 @@ class ProfilesController < Profiles::ApplicationController def update respond_to do |format| - result = Users::UpdateService.new(@user, user_params).execute + result = Users::UpdateService.new(current_user, @user, user_params).execute if result[:status] == :success message = "Profile was successfully updated" @@ -25,7 +25,7 @@ class ProfilesController < Profiles::ApplicationController end def reset_private_token - Users::UpdateService.new(@user).execute! do |user| + Users::UpdateService.new(current_user, @user).execute! do |user| user.reset_authentication_token! end @@ -35,7 +35,7 @@ class ProfilesController < Profiles::ApplicationController end def reset_incoming_email_token - Users::UpdateService.new(@user).execute! do |user| + Users::UpdateService.new(current_user, @user).execute! do |user| user.reset_incoming_email_token! end @@ -45,7 +45,7 @@ class ProfilesController < Profiles::ApplicationController end def reset_rss_token - Users::UpdateService.new(@user).execute! do |user| + Users::UpdateService.new(current_user, @user).execute! do |user| user.reset_rss_token! end @@ -61,7 +61,7 @@ class ProfilesController < Profiles::ApplicationController end def update_username - result = Users::UpdateService.new(@user, username: user_params[:username]).execute + result = Users::UpdateService.new(current_user, @user, username: user_params[:username]).execute options = if result[:status] == :success { notice: "Username successfully changed" } diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index be6491d042c..a0bd10d9726 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -55,7 +55,7 @@ class SessionsController < Devise::SessionsController return unless user && user.require_password_creation? - Users::UpdateService.new(user).execute do |user| + Users::UpdateService.new(current_user, user).execute do |user| @token = user.generate_reset_token end diff --git a/app/models/user.rb b/app/models/user.rb index 09c9b3250eb..e9a3aea44c4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -60,7 +60,7 @@ class User < ActiveRecord::Base lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i) return unless lease.try_obtain - Users::UpdateService.new(self).execute(validate: false) + Users::UpdateService.new(self, self).execute(validate: false) end attr_accessor :force_random_password @@ -526,8 +526,8 @@ class User < ActiveRecord::Base def update_emails_with_primary_email primary_email_record = emails.find_by(email: email) if primary_email_record - Emails::DestroyService.new(self, email: email).execute - Emails::CreateService.new(self, email: email_was).execute + Emails::DestroyService.new(self, self, email: email).execute + Emails::CreateService.new(self, self, email: email_was).execute end end @@ -1000,7 +1000,7 @@ class User < ActiveRecord::Base if attempts_exceeded? lock_access! unless access_locked? else - Users::UpdateService.new(self).execute(validate: false) + Users::UpdateService.new(self, self).execute(validate: false) end end @@ -1186,7 +1186,7 @@ class User < ActiveRecord::Base &creation_block ) - Users::UpdateService.new(user).execute(validate: false) + Users::UpdateService.new(user, user).execute(validate: false) user ensure Gitlab::ExclusiveLease.cancel(lease_key, uuid) diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb index d586b9dfe0c..02dd803fca6 100644 --- a/app/services/emails/destroy_service.rb +++ b/app/services/emails/destroy_service.rb @@ -1,13 +1,13 @@ module Emails class DestroyService < ::Emails::BaseService def execute - Email.find_by_email!(@email).destroy && update_secondary_emails! + update_secondary_emails! if Email.find_by_email!(@email).destroy end private def update_secondary_emails! - result = ::Users::UpdateService.new(@user).execute do |user| + result = ::Users::UpdateService.new(@current_user, @user).execute do |user| user.update_secondary_emails! end diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index 6188b8a4349..fddeeb9d52a 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -12,12 +12,8 @@ module Users assign_attributes(&block) - user_exists = @user.persisted? - if @user.save(validate: validate) - notify_new_user(@user, nil) unless user_exists - - success + notify_success else error(@user.errors.full_messages.uniq.join('. ')) end @@ -33,6 +29,12 @@ module Users private + def notify_success + notify_new_user(@user, nil) unless @user.persisted? + + success + end + def assign_attributes(&block) if @user.user_synced_attributes_metadata params.except!(*@user.user_synced_attributes_metadata.read_only_attributes) diff --git a/lib/api/internal.rb b/lib/api/internal.rb index c0fef56378f..3d331ffdce2 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -136,7 +136,7 @@ module API codes = nil - ::Users::UpdateService.new(user).execute! do |user| + ::Users::UpdateService.new(current_user, user).execute! do |user| codes = user.generate_otp_backup_codes! end diff --git a/lib/api/notification_settings.rb b/lib/api/notification_settings.rb index bcc0833aa5c..1c5592d952d 100644 --- a/lib/api/notification_settings.rb +++ b/lib/api/notification_settings.rb @@ -35,7 +35,7 @@ module API new_notification_email = params.delete(:notification_email) if new_notification_email - ::Users::UpdateService.new(current_user, notification_email: new_notification_email).execute + ::Users::UpdateService.new(current_user, current_user, notification_email: new_notification_email).execute end notification_setting.update(declared_params(include_missing: false)) diff --git a/lib/api/users.rb b/lib/api/users.rb index 77ac24ec68d..b157940eaa6 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -166,7 +166,7 @@ module API user_params[:password_expires_at] = Time.now if user_params[:password].present? - result = ::Users::UpdateService.new(user, user_params.except(:extern_uid, :provider)).execute + result = ::Users::UpdateService.new(current_user, user, user_params.except(:extern_uid, :provider)).execute if result[:status] == :success present user, with: Entities::UserPublic @@ -326,7 +326,7 @@ module API user = User.find_by(id: params.delete(:id)) not_found!('User') unless user - email = Emails::CreateService.new(user, declared_params(include_missing: false)).execute + email = Emails::CreateService.new(current_user, user, declared_params(include_missing: false)).execute if email.errors.blank? NotificationService.new.new_email(email) @@ -367,7 +367,7 @@ module API not_found!('Email') unless email destroy_conditionally!(email) do |email| - Emails::DestroyService.new(current_user, email: email.email).execute + Emails::DestroyService.new(current_user, user, email: email.email).execute end user.update_secondary_emails! @@ -672,7 +672,7 @@ module API requires :email, type: String, desc: 'The new email' end post "emails" do - email = Emails::CreateService.new(current_user, declared_params).execute + email = Emails::CreateService.new(current_user, current_user, declared_params).execute if email.errors.blank? NotificationService.new.new_email(email) @@ -691,7 +691,7 @@ module API not_found!('Email') unless email destroy_conditionally!(email) do |email| - Emails::DestroyService.new(current_user, email: email.email).execute + Emails::DestroyService.new(current_user, current_user, email: email.email).execute end current_user.update_secondary_emails! diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index fb68627dedf..4e75729ca73 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -16,7 +16,7 @@ module Gitlab def self.allowed?(user) self.open(user) do |access| if access.allowed? - Users::UpdateService.new(user, last_credential_check_at: Time.now).execute + Users::UpdateService.new(user, user, last_credential_check_at: Time.now).execute true else diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 7704bf715e4..eb32e41c995 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -32,7 +32,7 @@ module Gitlab block_after_save = needs_blocking? - Users::UpdateService.new(gl_user).execute! + Users::UpdateService.new(gl_user, gl_user).execute! gl_user.block if block_after_save -- cgit v1.2.1 From 7188975cb54a2b6902544340a91f94aa1fa82d3c Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 20 Sep 2017 12:00:33 +0200 Subject: update initializers --- app/services/emails/base_service.rb | 3 ++- app/services/users/update_service.rb | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb index ace49889097..8810f6d8803 100644 --- a/app/services/emails/base_service.rb +++ b/app/services/emails/base_service.rb @@ -1,6 +1,7 @@ module Emails class BaseService - def initialize(user, opts) + def initialize(current_user, user, opts) + @current_user = current_user @user = user @email = opts[:email] end diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index fddeeb9d52a..05cb1655325 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -2,7 +2,8 @@ module Users class UpdateService < BaseService include NewUserNotifier - def initialize(user, params = {}) + def initialize(current_user, user, params = {}) + @current_user = current_user @user = user @params = params.dup end -- cgit v1.2.1 From faa95ba4fbeb93e6af19770f7aade54aa9d30304 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 20 Sep 2017 12:31:35 +0200 Subject: fix users update service --- app/services/users/update_service.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index 05cb1655325..9f013af38dc 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -11,10 +11,12 @@ module Users def execute(validate: true, &block) yield(@user) if block_given? + user_exists = @user.persisted? + assign_attributes(&block) if @user.save(validate: validate) - notify_success + notify_success(user_exists) else error(@user.errors.full_messages.uniq.join('. ')) end @@ -30,8 +32,8 @@ module Users private - def notify_success - notify_new_user(@user, nil) unless @user.persisted? + def notify_success(user_exists) + notify_new_user(@user, nil) unless user_exists success end -- cgit v1.2.1 From f2e9ef102713344938e425d68d1a017403a710e0 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 20 Sep 2017 13:36:00 +0200 Subject: fix specs --- spec/services/emails/create_service_spec.rb | 2 +- spec/services/emails/destroy_service_spec.rb | 2 +- spec/services/users/update_service_spec.rb | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb index 641d5538de8..1c8b81c222c 100644 --- a/spec/services/emails/create_service_spec.rb +++ b/spec/services/emails/create_service_spec.rb @@ -4,7 +4,7 @@ describe Emails::CreateService do let(:user) { create(:user) } let(:opts) { { email: 'new@email.com' } } - subject(:service) { described_class.new(user, opts) } + subject(:service) { described_class.new(user, user, opts) } describe '#execute' do it 'creates an email with valid attributes' do diff --git a/spec/services/emails/destroy_service_spec.rb b/spec/services/emails/destroy_service_spec.rb index 1f4294dd905..138c3ff33b0 100644 --- a/spec/services/emails/destroy_service_spec.rb +++ b/spec/services/emails/destroy_service_spec.rb @@ -4,7 +4,7 @@ describe Emails::DestroyService do let!(:user) { create(:user) } let!(:email) { create(:email, user: user) } - subject(:service) { described_class.new(user, email: email.email) } + subject(:service) { described_class.new(user, user, email: email.email) } describe '#execute' do it 'removes an email' do diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb index 6ee35a33b2d..707f83b3359 100644 --- a/spec/services/users/update_service_spec.rb +++ b/spec/services/users/update_service_spec.rb @@ -31,13 +31,13 @@ describe Users::UpdateService do end def update_user(user, opts) - described_class.new(user, opts).execute + described_class.new(user, user, opts).execute end end describe '#execute!' do it 'updates the name' do - service = described_class.new(user, name: 'New Name') + service = described_class.new(user, user, name: 'New Name') expect(service).not_to receive(:notify_new_user) result = service.execute! @@ -55,7 +55,7 @@ describe Users::UpdateService do it 'fires system hooks when a new user is saved' do system_hook_service = spy(:system_hook_service) user = build(:user) - service = described_class.new(user, name: 'John Doe') + service = described_class.new(user, user, name: 'John Doe') expect(service).to receive(:notify_new_user).and_call_original expect(service).to receive(:system_hook_service).and_return(system_hook_service) @@ -65,7 +65,7 @@ describe Users::UpdateService do end def update_user(user, opts) - described_class.new(user, opts).execute! + described_class.new(user, user, opts).execute! end end end -- cgit v1.2.1 From 4a6ec7c947b8ae5360b220ec8c6c91b2221d2091 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 21 Sep 2017 10:29:09 +0200 Subject: refactor some controllers to make them EE friendly --- app/controllers/admin/applications_controller.rb | 10 ++++++++-- app/controllers/confirmations_controller.rb | 6 +++++- app/controllers/oauth/applications_controller.rb | 10 ++++++++-- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb index 16590e66d61..fb6d8c0bb81 100644 --- a/app/controllers/admin/applications_controller.rb +++ b/app/controllers/admin/applications_controller.rb @@ -22,8 +22,7 @@ class Admin::ApplicationsController < Admin::ApplicationController @application = Doorkeeper::Application.new(application_params) if @application.save - flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create]) - redirect_to admin_application_url(@application) + redirect_to_admin_page else render :new end @@ -42,6 +41,13 @@ class Admin::ApplicationsController < Admin::ApplicationController redirect_to admin_applications_url, status: 302, notice: 'Application was successfully destroyed.' end + protected + + def redirect_to_admin_page + flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create]) + redirect_to admin_application_url(@application) + end + private def set_application diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb index 306afb65f10..10d2665c06a 100644 --- a/app/controllers/confirmations_controller.rb +++ b/app/controllers/confirmations_controller.rb @@ -12,10 +12,14 @@ class ConfirmationsController < Devise::ConfirmationsController def after_confirmation_path_for(resource_name, resource) if signed_in?(resource_name) - after_sign_in_path_for(resource) + after_sign_in(resource) else flash[:notice] += " Please sign in." new_session_path(resource_name) end end + + def after_sign_in(resource) + after_sign_in_path_for(resource) + end end diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb index 2ae4785b12c..b02e64a132b 100644 --- a/app/controllers/oauth/applications_controller.rb +++ b/app/controllers/oauth/applications_controller.rb @@ -21,14 +21,20 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController @application.owner = current_user if @application.save - flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create]) - redirect_to oauth_application_url(@application) + redirect_to_oauth_application_page else set_index_vars render :index end end + protected + + def redirect_to_oauth_application_page + flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create]) + redirect_to oauth_application_url(@application) + end + private def verify_user_oauth_applications_enabled -- cgit v1.2.1 From cbb90d8f84d1f79560066d8ea3c6346906e7da94 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 21 Sep 2017 10:55:24 +0200 Subject: refactor keys controller --- app/controllers/profiles/keys_controller.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/controllers/profiles/keys_controller.rb b/app/controllers/profiles/keys_controller.rb index 89d6d7f1b52..069e6a810f2 100644 --- a/app/controllers/profiles/keys_controller.rb +++ b/app/controllers/profiles/keys_controller.rb @@ -14,7 +14,7 @@ class Profiles::KeysController < Profiles::ApplicationController @key = Keys::CreateService.new(current_user, key_params).execute if @key.persisted? - redirect_to profile_key_path(@key) + redirect_to_profile_key_path else @keys = current_user.keys.select(&:persisted?) render :index @@ -50,6 +50,12 @@ class Profiles::KeysController < Profiles::ApplicationController end end + protected + + def redirect_to_profile_key_path + redirect_to profile_key_path(@key) + end + private def key_params -- cgit v1.2.1 From 67d06dee30379fc93be196e2cf509660d1661aea Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 27 Sep 2017 11:48:33 +0200 Subject: refactor users update service --- app/controllers/admin/users_controller.rb | 4 ++-- app/controllers/profiles/avatars_controller.rb | 2 +- app/controllers/profiles/notifications_controller.rb | 2 +- app/controllers/profiles/passwords_controller.rb | 6 +++--- app/controllers/profiles/preferences_controller.rb | 2 +- app/controllers/profiles/two_factor_auths_controller.rb | 6 +++--- app/controllers/profiles_controller.rb | 10 +++++----- app/controllers/sessions_controller.rb | 2 +- app/models/user.rb | 6 +++--- app/services/emails/destroy_service.rb | 2 +- app/services/users/update_service.rb | 4 ++-- lib/api/internal.rb | 2 +- lib/api/notification_settings.rb | 2 +- lib/api/users.rb | 2 +- lib/gitlab/ldap/access.rb | 2 +- lib/gitlab/o_auth/user.rb | 2 +- spec/services/users/update_service_spec.rb | 8 ++++---- 17 files changed, 32 insertions(+), 32 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 8b6119548ee..b25040382d8 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -128,7 +128,7 @@ class Admin::UsersController < Admin::ApplicationController end respond_to do |format| - result = Users::UpdateService.new(current_user, user, user_params_with_pass).execute do |user| + result = Users::UpdateService.new(current_user, user_params_with_pass.merge(user: user)).execute do |user| user.skip_reconfirmation! end @@ -219,7 +219,7 @@ class Admin::UsersController < Admin::ApplicationController end def update_user(&block) - result = Users::UpdateService.new(current_user, user).execute(&block) + result = Users::UpdateService.new(current_user, user: user).execute(&block) result[:status] == :success end diff --git a/app/controllers/profiles/avatars_controller.rb b/app/controllers/profiles/avatars_controller.rb index 5a94dc88455..39b9f8a84d1 100644 --- a/app/controllers/profiles/avatars_controller.rb +++ b/app/controllers/profiles/avatars_controller.rb @@ -2,7 +2,7 @@ class Profiles::AvatarsController < Profiles::ApplicationController def destroy @user = current_user - Users::UpdateService.new(current_user, @user).execute { |user| user.remove_avatar! } + Users::UpdateService.new(current_user, user: @user).execute { |user| user.remove_avatar! } redirect_to profile_path, status: 302 end diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb index 45d7bca27bb..8a38ba65d4c 100644 --- a/app/controllers/profiles/notifications_controller.rb +++ b/app/controllers/profiles/notifications_controller.rb @@ -7,7 +7,7 @@ class Profiles::NotificationsController < Profiles::ApplicationController end def update - result = Users::UpdateService.new(current_user, current_user, user_params).execute + result = Users::UpdateService.new(current_user, user_params.merge(user: current_user)).execute if result[:status] == :success flash[:notice] = "Notification settings saved" diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb index 08b438de9e2..dcfcb855ab5 100644 --- a/app/controllers/profiles/passwords_controller.rb +++ b/app/controllers/profiles/passwords_controller.rb @@ -21,10 +21,10 @@ class Profiles::PasswordsController < Profiles::ApplicationController password_automatically_set: false } - result = Users::UpdateService.new(current_user, @user, password_attributes).execute + result = Users::UpdateService.new(current_user, password_attributes.merge(user: @user)).execute if result[:status] == :success - Users::UpdateService.new(current_user, @user, password_expires_at: nil).execute + Users::UpdateService.new(current_user, user: @user, password_expires_at: nil).execute redirect_to root_path, notice: 'Password successfully changed' else @@ -46,7 +46,7 @@ class Profiles::PasswordsController < Profiles::ApplicationController return end - result = Users::UpdateService.new(current_user, @user, password_attributes).execute + result = Users::UpdateService.new(current_user, password_attributes.merge(user: @user)).execute if result[:status] == :success flash[:notice] = "Password was successfully updated. Please login with it" diff --git a/app/controllers/profiles/preferences_controller.rb b/app/controllers/profiles/preferences_controller.rb index a13b9a616cb..ed0f98179eb 100644 --- a/app/controllers/profiles/preferences_controller.rb +++ b/app/controllers/profiles/preferences_controller.rb @@ -6,7 +6,7 @@ class Profiles::PreferencesController < Profiles::ApplicationController def update begin - result = Users::UpdateService.new(current_user, user, preferences_params).execute + result = Users::UpdateService.new(current_user, preferences_params.merge(user: user)).execute if result[:status] == :success flash[:notice] = 'Preferences saved.' diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index b590846257b..aa9789f8a0f 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -10,7 +10,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController current_user.otp_grace_period_started_at = Time.current end - Users::UpdateService.new(current_user, current_user).execute! + Users::UpdateService.new(current_user, user: current_user).execute! if two_factor_authentication_required? && !current_user.two_factor_enabled? two_factor_authentication_reason( @@ -41,7 +41,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController def create if current_user.validate_and_consume_otp!(params[:pin_code]) - Users::UpdateService.new(current_user, current_user, otp_required_for_login: true).execute! do |user| + Users::UpdateService.new(current_user, user: current_user, otp_required_for_login: true).execute! do |user| @codes = user.generate_otp_backup_codes! end @@ -70,7 +70,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController end def codes - Users::UpdateService.new(current_user, current_user).execute! do |user| + Users::UpdateService.new(current_user, user: current_user).execute! do |user| @codes = user.generate_otp_backup_codes! end end diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index 9e85997cf6d..5d87037f012 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -10,7 +10,7 @@ class ProfilesController < Profiles::ApplicationController def update respond_to do |format| - result = Users::UpdateService.new(current_user, @user, user_params).execute + result = Users::UpdateService.new(current_user, user_params.merge(user: @user)).execute if result[:status] == :success message = "Profile was successfully updated" @@ -25,7 +25,7 @@ class ProfilesController < Profiles::ApplicationController end def reset_private_token - Users::UpdateService.new(current_user, @user).execute! do |user| + Users::UpdateService.new(current_user, user: @user).execute! do |user| user.reset_authentication_token! end @@ -35,7 +35,7 @@ class ProfilesController < Profiles::ApplicationController end def reset_incoming_email_token - Users::UpdateService.new(current_user, @user).execute! do |user| + Users::UpdateService.new(current_user, user: @user).execute! do |user| user.reset_incoming_email_token! end @@ -45,7 +45,7 @@ class ProfilesController < Profiles::ApplicationController end def reset_rss_token - Users::UpdateService.new(current_user, @user).execute! do |user| + Users::UpdateService.new(current_user, user: @user).execute! do |user| user.reset_rss_token! end @@ -61,7 +61,7 @@ class ProfilesController < Profiles::ApplicationController end def update_username - result = Users::UpdateService.new(current_user, @user, username: user_params[:username]).execute + result = Users::UpdateService.new(current_user, user: @user, username: user_params[:username]).execute options = if result[:status] == :success { notice: "Username successfully changed" } diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index a0bd10d9726..fe3bb117410 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -55,7 +55,7 @@ class SessionsController < Devise::SessionsController return unless user && user.require_password_creation? - Users::UpdateService.new(current_user, user).execute do |user| + Users::UpdateService.new(current_user, user: user).execute do |user| @token = user.generate_reset_token end diff --git a/app/models/user.rb b/app/models/user.rb index e9a3aea44c4..94674528012 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -60,7 +60,7 @@ class User < ActiveRecord::Base lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i) return unless lease.try_obtain - Users::UpdateService.new(self, self).execute(validate: false) + Users::UpdateService.new(self, user: self).execute(validate: false) end attr_accessor :force_random_password @@ -1000,7 +1000,7 @@ class User < ActiveRecord::Base if attempts_exceeded? lock_access! unless access_locked? else - Users::UpdateService.new(self, self).execute(validate: false) + Users::UpdateService.new(self, user: self).execute(validate: false) end end @@ -1186,7 +1186,7 @@ class User < ActiveRecord::Base &creation_block ) - Users::UpdateService.new(user, user).execute(validate: false) + Users::UpdateService.new(user, user: user).execute(validate: false) user ensure Gitlab::ExclusiveLease.cancel(lease_key, uuid) diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb index 02dd803fca6..44011cc36c8 100644 --- a/app/services/emails/destroy_service.rb +++ b/app/services/emails/destroy_service.rb @@ -7,7 +7,7 @@ module Emails private def update_secondary_emails! - result = ::Users::UpdateService.new(@current_user, @user).execute do |user| + result = ::Users::UpdateService.new(@current_user, user: @user).execute do |user| user.update_secondary_emails! end diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index 9f013af38dc..e2e720a5703 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -2,9 +2,9 @@ module Users class UpdateService < BaseService include NewUserNotifier - def initialize(current_user, user, params = {}) + def initialize(current_user, params = {}) @current_user = current_user - @user = user + @user = params[:user] @params = params.dup end diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 3d331ffdce2..a0557a609ca 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -136,7 +136,7 @@ module API codes = nil - ::Users::UpdateService.new(current_user, user).execute! do |user| + ::Users::UpdateService.new(current_user, user: user).execute! do |user| codes = user.generate_otp_backup_codes! end diff --git a/lib/api/notification_settings.rb b/lib/api/notification_settings.rb index 1c5592d952d..0266bf2f717 100644 --- a/lib/api/notification_settings.rb +++ b/lib/api/notification_settings.rb @@ -35,7 +35,7 @@ module API new_notification_email = params.delete(:notification_email) if new_notification_email - ::Users::UpdateService.new(current_user, current_user, notification_email: new_notification_email).execute + ::Users::UpdateService.new(current_user, user: current_user, notification_email: new_notification_email).execute end notification_setting.update(declared_params(include_missing: false)) diff --git a/lib/api/users.rb b/lib/api/users.rb index b157940eaa6..ce5d75965d5 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -166,7 +166,7 @@ module API user_params[:password_expires_at] = Time.now if user_params[:password].present? - result = ::Users::UpdateService.new(current_user, user, user_params.except(:extern_uid, :provider)).execute + result = ::Users::UpdateService.new(current_user, user_params.except(:extern_uid, :provider).merge(user: user)).execute if result[:status] == :success present user, with: Entities::UserPublic diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 4e75729ca73..e60ceba27c8 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -16,7 +16,7 @@ module Gitlab def self.allowed?(user) self.open(user) do |access| if access.allowed? - Users::UpdateService.new(user, user, last_credential_check_at: Time.now).execute + Users::UpdateService.new(user, user: user, last_credential_check_at: Time.now).execute true else diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index eb32e41c995..e06d4dc45f7 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -32,7 +32,7 @@ module Gitlab block_after_save = needs_blocking? - Users::UpdateService.new(gl_user, gl_user).execute! + Users::UpdateService.new(gl_user, user: gl_user).execute! gl_user.block if block_after_save diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb index 707f83b3359..f8d4a47b212 100644 --- a/spec/services/users/update_service_spec.rb +++ b/spec/services/users/update_service_spec.rb @@ -31,13 +31,13 @@ describe Users::UpdateService do end def update_user(user, opts) - described_class.new(user, user, opts).execute + described_class.new(user, opts.merge(user: user)).execute end end describe '#execute!' do it 'updates the name' do - service = described_class.new(user, user, name: 'New Name') + service = described_class.new(user, user: user, name: 'New Name') expect(service).not_to receive(:notify_new_user) result = service.execute! @@ -55,7 +55,7 @@ describe Users::UpdateService do it 'fires system hooks when a new user is saved' do system_hook_service = spy(:system_hook_service) user = build(:user) - service = described_class.new(user, user, name: 'John Doe') + service = described_class.new(user, user: user, name: 'John Doe') expect(service).to receive(:notify_new_user).and_call_original expect(service).to receive(:system_hook_service).and_return(system_hook_service) @@ -65,7 +65,7 @@ describe Users::UpdateService do end def update_user(user, opts) - described_class.new(user, user, opts).execute! + described_class.new(user, opts.merge(user: user)).execute! end end end -- cgit v1.2.1 From e07819cbf70511ad38c107dc593ec87606567cdc Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 27 Sep 2017 12:08:10 +0200 Subject: fix update service --- app/services/users/update_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index e2e720a5703..15ca1a55a5b 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -4,7 +4,7 @@ module Users def initialize(current_user, params = {}) @current_user = current_user - @user = params[:user] + @user = params.delete(:user) @params = params.dup end -- cgit v1.2.1 From 1dcb7111107ed5a6b6258613d804b8da56af8b35 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 27 Sep 2017 16:39:10 +0200 Subject: refactor emails service --- app/controllers/admin/users_controller.rb | 2 +- app/controllers/profiles/emails_controller.rb | 4 ++-- app/models/user.rb | 4 ++-- app/services/emails/base_service.rb | 4 ++-- lib/api/users.rb | 8 ++++---- spec/services/emails/create_service_spec.rb | 4 ++-- spec/services/emails/destroy_service_spec.rb | 2 +- 7 files changed, 14 insertions(+), 14 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index b25040382d8..676a7203c7d 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -155,7 +155,7 @@ class Admin::UsersController < Admin::ApplicationController def remove_email email = user.emails.find(params[:email_id]) - success = Emails::DestroyService.new(current_user, user, email: email.email).execute + success = Emails::DestroyService.new(current_user, user: user, email: email.email).execute respond_to do |format| if success diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb index 8328d230276..97db84b92d4 100644 --- a/app/controllers/profiles/emails_controller.rb +++ b/app/controllers/profiles/emails_controller.rb @@ -5,7 +5,7 @@ class Profiles::EmailsController < Profiles::ApplicationController end def create - @email = Emails::CreateService.new(current_user, current_user, email_params).execute + @email = Emails::CreateService.new(current_user, email_params.merge(user: current_user)).execute if @email.errors.blank? NotificationService.new.new_email(@email) @@ -19,7 +19,7 @@ class Profiles::EmailsController < Profiles::ApplicationController def destroy @email = current_user.emails.find(params[:id]) - Emails::DestroyService.new(current_user, current_user, email: @email.email).execute + Emails::DestroyService.new(current_user, user: current_user, email: @email.email).execute respond_to do |format| format.html { redirect_to profile_emails_url, status: 302 } diff --git a/app/models/user.rb b/app/models/user.rb index 94674528012..103ac78783f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -526,8 +526,8 @@ class User < ActiveRecord::Base def update_emails_with_primary_email primary_email_record = emails.find_by(email: email) if primary_email_record - Emails::DestroyService.new(self, self, email: email).execute - Emails::CreateService.new(self, self, email: email_was).execute + Emails::DestroyService.new(self, user: self, email: email).execute + Emails::CreateService.new(self, user: self, email: email_was).execute end end diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb index 8810f6d8803..7f591c89411 100644 --- a/app/services/emails/base_service.rb +++ b/app/services/emails/base_service.rb @@ -1,8 +1,8 @@ module Emails class BaseService - def initialize(current_user, user, opts) + def initialize(current_user, opts) @current_user = current_user - @user = user + @user = opts.delete(:user) @email = opts[:email] end end diff --git a/lib/api/users.rb b/lib/api/users.rb index ce5d75965d5..99024d1f0ad 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -326,7 +326,7 @@ module API user = User.find_by(id: params.delete(:id)) not_found!('User') unless user - email = Emails::CreateService.new(current_user, user, declared_params(include_missing: false)).execute + email = Emails::CreateService.new(current_user, declared_params(include_missing: false).merge(user: user)).execute if email.errors.blank? NotificationService.new.new_email(email) @@ -367,7 +367,7 @@ module API not_found!('Email') unless email destroy_conditionally!(email) do |email| - Emails::DestroyService.new(current_user, user, email: email.email).execute + Emails::DestroyService.new(current_user, user: user, email: email.email).execute end user.update_secondary_emails! @@ -672,7 +672,7 @@ module API requires :email, type: String, desc: 'The new email' end post "emails" do - email = Emails::CreateService.new(current_user, current_user, declared_params).execute + email = Emails::CreateService.new(current_user, declared_params.merge(user: current_user)).execute if email.errors.blank? NotificationService.new.new_email(email) @@ -691,7 +691,7 @@ module API not_found!('Email') unless email destroy_conditionally!(email) do |email| - Emails::DestroyService.new(current_user, current_user, email: email.email).execute + Emails::DestroyService.new(current_user, user: current_user, email: email.email).execute end current_user.update_secondary_emails! diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb index 1c8b81c222c..75812c17309 100644 --- a/spec/services/emails/create_service_spec.rb +++ b/spec/services/emails/create_service_spec.rb @@ -2,9 +2,9 @@ require 'spec_helper' describe Emails::CreateService do let(:user) { create(:user) } - let(:opts) { { email: 'new@email.com' } } + let(:opts) { { email: 'new@email.com', user: user } } - subject(:service) { described_class.new(user, user, opts) } + subject(:service) { described_class.new(user, opts) } describe '#execute' do it 'creates an email with valid attributes' do diff --git a/spec/services/emails/destroy_service_spec.rb b/spec/services/emails/destroy_service_spec.rb index 138c3ff33b0..7726fc0ef81 100644 --- a/spec/services/emails/destroy_service_spec.rb +++ b/spec/services/emails/destroy_service_spec.rb @@ -4,7 +4,7 @@ describe Emails::DestroyService do let!(:user) { create(:user) } let!(:email) { create(:email, user: user) } - subject(:service) { described_class.new(user, user, email: email.email) } + subject(:service) { described_class.new(user, user: user, email: email.email) } describe '#execute' do it 'removes an email' do -- cgit v1.2.1