diff options
33 files changed, 331 insertions, 1006 deletions
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index f020e0af588..ca594b71054 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1483,6 +1483,7 @@ Rails/SaveBang: - 'spec/support/shared_examples/models/members_notifications_shared_example.rb' - 'spec/support/shared_examples/models/mentionable_shared_examples.rb' - 'spec/support/shared_examples/models/project_latest_successful_build_for_shared_examples.rb' + - 'spec/support/shared_examples/models/relative_positioning_shared_examples.rb' - 'spec/support/shared_examples/models/slack_mattermost_notifications_shared_examples.rb' - 'spec/support/shared_examples/models/update_project_statistics_shared_examples.rb' - 'spec/support/shared_examples/models/with_uploads_shared_examples.rb' diff --git a/app/assets/javascripts/design_management/components/delete_button.vue b/app/assets/javascripts/design_management/components/delete_button.vue index 83a3cb56b83..37686dd5a46 100644 --- a/app/assets/javascripts/design_management/components/delete_button.vue +++ b/app/assets/javascripts/design_management/components/delete_button.vue @@ -95,11 +95,9 @@ export default { :category="buttonCategory" :size="buttonSize" :class="buttonClass" - :disabled="isDeleting || !hasSelectedDesigns" :loading="loading" :icon="buttonIcon" - > - <slot></slot> - </gl-button> + :disabled="isDeleting || !hasSelectedDesigns" + /> </div> </template> diff --git a/app/assets/javascripts/design_management/components/toolbar/pagination.vue b/app/assets/javascripts/design_management/components/toolbar/design_navigation.vue index 222f9051a11..afca8ed2c6f 100644 --- a/app/assets/javascripts/design_management/components/toolbar/pagination.vue +++ b/app/assets/javascripts/design_management/components/toolbar/design_navigation.vue @@ -1,14 +1,15 @@ <script> /* global Mousetrap */ import 'mousetrap'; +import { GlButton, GlButtonGroup } from '@gitlab/ui'; import { s__, sprintf } from '~/locale'; -import PaginationButton from './pagination_button.vue'; import allDesignsMixin from '../../mixins/all_designs'; import { DESIGN_ROUTE_NAME } from '../../router/constants'; export default { components: { - PaginationButton, + GlButton, + GlButtonGroup, }, mixins: [allDesignsMixin], props: { @@ -31,12 +32,12 @@ export default { }); }, previousDesign() { - if (!this.designsCount) return null; + if (this.currentIndex === 0) return null; return this.designs[this.currentIndex - 1]; }, nextDesign() { - if (!this.designsCount) return null; + if (this.currentIndex + 1 === this.designsCount) return null; return this.designs[this.currentIndex + 1]; }, @@ -65,19 +66,21 @@ export default { <template> <div v-if="designsCount" class="d-flex align-items-center"> {{ paginationText }} - <div class="btn-group gl-ml-3"> - <pagination-button - :design="previousDesign" + <gl-button-group class="ml-3 mr-3"> + <gl-button + :disabled="!previousDesign" :title="s__('DesignManagement|Go to previous design')" - icon-name="angle-left" + icon="angle-left" class="js-previous-design" + @click="navigateToDesign(previousDesign)" /> - <pagination-button - :design="nextDesign" + <gl-button + :disabled="!nextDesign" :title="s__('DesignManagement|Go to next design')" - icon-name="angle-right" + icon="angle-right" class="js-next-design" + @click="navigateToDesign(nextDesign)" /> - </div> + </gl-button-group> </div> </template> diff --git a/app/assets/javascripts/design_management/components/toolbar/index.vue b/app/assets/javascripts/design_management/components/toolbar/index.vue index 3b113320aa5..a1cb57123ab 100644 --- a/app/assets/javascripts/design_management/components/toolbar/index.vue +++ b/app/assets/javascripts/design_management/components/toolbar/index.vue @@ -2,17 +2,17 @@ import { GlButton, GlIcon } from '@gitlab/ui'; import { __, sprintf } from '~/locale'; import timeagoMixin from '~/vue_shared/mixins/timeago'; -import Pagination from './pagination.vue'; +import DesignNavigation from './design_navigation.vue'; import DeleteButton from '../delete_button.vue'; import permissionsQuery from '../../graphql/queries/design_permissions.query.graphql'; import { DESIGNS_ROUTE_NAME } from '../../router/constants'; export default { components: { + GlButton, GlIcon, - Pagination, + DesignNavigation, DeleteButton, - GlButton, }, mixins: [timeagoMixin], props: { @@ -111,19 +111,16 @@ export default { <small v-if="updatedAt" class="text-secondary">{{ updatedText }}</small> </div> </div> - - <div class="gl-display-flex gl-align-items-center"> - <pagination :id="id" class="gl-mr-3 gl-flex-shrink-0" /> - <gl-button :href="image" icon="download" /> - <delete-button - v-if="isLatestVersion && canDeleteDesign" - class="gl-ml-3" - :is-deleting="isDeleting" - button-variant="warning" - button-icon="archive" - button-category="secondary" - @deleteSelectedDesigns="$emit('delete')" - /> - </div> + <design-navigation :id="id" class="ml-auto flex-shrink-0" /> + <gl-button :href="image" icon="download" /> + <delete-button + v-if="isLatestVersion && canDeleteDesign" + class="gl-ml-3" + :is-deleting="isDeleting" + button-variant="warning" + button-icon="archive" + button-category="secondary" + @deleteSelectedDesigns="$emit('delete')" + /> </header> </template> diff --git a/app/assets/javascripts/design_management/components/toolbar/pagination_button.vue b/app/assets/javascripts/design_management/components/toolbar/pagination_button.vue deleted file mode 100644 index f00ecefca01..00000000000 --- a/app/assets/javascripts/design_management/components/toolbar/pagination_button.vue +++ /dev/null @@ -1,48 +0,0 @@ -<script> -import Icon from '~/vue_shared/components/icon.vue'; -import { DESIGN_ROUTE_NAME } from '../../router/constants'; - -export default { - components: { - Icon, - }, - props: { - design: { - type: Object, - required: false, - default: null, - }, - title: { - type: String, - required: true, - }, - iconName: { - type: String, - required: true, - }, - }, - computed: { - designLink() { - if (!this.design) return {}; - - return { - name: DESIGN_ROUTE_NAME, - params: { id: this.design.filename }, - query: this.$route.query, - }; - }, - }, -}; -</script> - -<template> - <router-link - :to="designLink" - :disabled="!design" - :class="{ disabled: !design }" - :aria-label="title" - class="btn btn-default" - > - <icon :name="iconName" /> - </router-link> -</template> diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb index afc818b28ab..ca64907f04b 100644 --- a/app/models/concerns/relative_positioning.rb +++ b/app/models/concerns/relative_positioning.rb @@ -3,15 +3,11 @@ # This module makes it possible to handle items as a list, where the order of items can be easily altered # Requirements: # -# The model must have the following named columns: -# - id: integer -# - relative_position: integer +# - Only works for ActiveRecord models +# - relative_position integer field must present on the model +# - This module uses GROUP BY: the model should have a parent relation, example: project -> issues, project is the parent relation (issues table has a parent_id column) # -# The model must support a concept of siblings via a child->parent relationship, -# to enable rebalancing and `GROUP BY` in queries. -# - example: project -> issues, project is the parent relation (issues table has a parent_id column) -# -# Two class methods must be defined when including this concern: +# Setup like this in the body of your class: # # include RelativePositioning # @@ -28,162 +24,66 @@ module RelativePositioning extend ActiveSupport::Concern - STEPS = 10 - IDEAL_DISTANCE = 2**(STEPS - 1) + 1 - - MIN_POSITION = Gitlab::Database::MIN_INT_VALUE - START_POSITION = 0 + MIN_POSITION = 0 + START_POSITION = Gitlab::Database::MAX_INT_VALUE / 2 MAX_POSITION = Gitlab::Database::MAX_INT_VALUE - - MAX_GAP = IDEAL_DISTANCE * 2 - MIN_GAP = 2 - - NoSpaceLeft = Class.new(StandardError) + IDEAL_DISTANCE = 500 class_methods do def move_nulls_to_end(objects) - move_nulls(objects, at_end: true) + objects = objects.reject(&:relative_position) + return if objects.empty? + + self.transaction do + max_relative_position = objects.first.max_relative_position + + objects.each do |object| + relative_position = position_between(max_relative_position || START_POSITION, MAX_POSITION) + object.update_column(:relative_position, relative_position) + + max_relative_position = relative_position + end + end end def move_nulls_to_start(objects) - move_nulls(objects, at_end: false) + objects = objects.reject(&:relative_position) + return if objects.empty? + + self.transaction do + min_relative_position = objects.first.min_relative_position + + objects.reverse_each do |object| + relative_position = position_between(MIN_POSITION, min_relative_position || START_POSITION) + object.update_column(:relative_position, relative_position) + + min_relative_position = relative_position + end + end end # This method takes two integer values (positions) and # calculates the position between them. The range is huge as - # the maximum integer value is 2147483647. - # - # We avoid open ranges by clamping the range to [MIN_POSITION, MAX_POSITION]. - # - # Then we handle one of three cases: - # - If the gap is too small, we raise NoSpaceLeft - # - If the gap is larger than MAX_GAP, we place the new position at most - # IDEAL_DISTANCE from the edge of the gap. - # - otherwise we place the new position at the midpoint. - # - # The new position will always satisfy: pos_before <= midpoint <= pos_after - # - # As a precondition, the gap between pos_before and pos_after MUST be >= 2. - # If the gap is too small, NoSpaceLeft is raised. - # - # This class method should only be called by instance methods of this module, which - # include handling for minimum gap size. - # - # @raises NoSpaceLeft - # @api private + # the maximum integer value is 2147483647. We are incrementing position by IDEAL_DISTANCE * 2 every time + # when we have enough space. If distance is less than IDEAL_DISTANCE, we are calculating an average number. def position_between(pos_before, pos_after) pos_before ||= MIN_POSITION pos_after ||= MAX_POSITION pos_before, pos_after = [pos_before, pos_after].sort - gap_width = pos_after - pos_before - midpoint = [pos_after - 1, pos_before + (gap_width / 2)].min + halfway = (pos_after + pos_before) / 2 + distance_to_halfway = pos_after - halfway - if gap_width < MIN_GAP - raise NoSpaceLeft - elsif gap_width > MAX_GAP + if distance_to_halfway < IDEAL_DISTANCE + halfway + else if pos_before == MIN_POSITION pos_after - IDEAL_DISTANCE elsif pos_after == MAX_POSITION pos_before + IDEAL_DISTANCE else - midpoint - end - else - midpoint - end - end - - private - - # @api private - def gap_size(object, gaps:, at_end:, starting_from:) - total_width = IDEAL_DISTANCE * gaps - size = if at_end && starting_from + total_width >= MAX_POSITION - (MAX_POSITION - starting_from) / gaps - elsif !at_end && starting_from - total_width <= MIN_POSITION - (starting_from - MIN_POSITION) / gaps - else - IDEAL_DISTANCE - end - - # Shift max elements leftwards if there isn't enough space - return [size, starting_from] if size >= MIN_GAP - - order = at_end ? :desc : :asc - terminus = object - .send(:relative_siblings) # rubocop:disable GitlabSecurity/PublicSend - .where('relative_position IS NOT NULL') - .order(relative_position: order) - .first - - if at_end - terminus.move_sequence_before(true) - max_relative_position = terminus.reset.relative_position - [[(MAX_POSITION - max_relative_position) / gaps, IDEAL_DISTANCE].min, max_relative_position] - else - terminus.move_sequence_after(true) - min_relative_position = terminus.reset.relative_position - [[(min_relative_position - MIN_POSITION) / gaps, IDEAL_DISTANCE].min, min_relative_position] - end - end - - # @api private - # @param [Array<RelativePositioning>] objects The objects to give positions to. The relative - # order will be preserved (i.e. when this method returns, - # objects.first.relative_position < objects.last.relative_position) - # @param [Boolean] at_end: The placement. - # If `true`, then all objects with `null` positions are placed _after_ - # all siblings with positions. If `false`, all objects with `null` - # positions are placed _before_ all siblings with positions. - def move_nulls(objects, at_end:) - objects = objects.reject(&:relative_position) - return if objects.empty? - - representative = objects.first - number_of_gaps = objects.size + 1 # 1 at left, one between each, and one at right - position = if at_end - representative.max_relative_position - else - representative.min_relative_position - end - - position ||= START_POSITION # If there are no positioned siblings, start from START_POSITION - - gap, position = gap_size(representative, gaps: number_of_gaps, at_end: at_end, starting_from: position) - - # Raise if we could not make enough space - raise NoSpaceLeft if gap < MIN_GAP - - indexed = objects.each_with_index.to_a - starting_from = at_end ? position : position - (gap * number_of_gaps) - - # Some classes are polymorphic, and not all siblings are in the same table. - by_model = indexed.group_by { |pair| pair.first.class } - - by_model.each do |model, pairs| - model.transaction do - pairs.each_slice(100) do |batch| - # These are known to be integers, one from the DB, and the other - # calculated by us, and thus safe to interpolate - values = batch.map do |obj, i| - pos = starting_from + gap * (i + 1) - obj.relative_position = pos - "(#{obj.id}, #{pos})" - end.join(', ') - - model.connection.exec_query(<<~SQL, "UPDATE #{model.table_name} positions") - WITH cte(cte_id, new_pos) AS ( - SELECT * - FROM (VALUES #{values}) as t (id, pos) - ) - UPDATE #{model.table_name} - SET relative_position = cte.new_pos - FROM cte - WHERE cte_id = id - SQL - end + halfway end end end @@ -197,12 +97,11 @@ module RelativePositioning calculate_relative_position('MAX', &block) end - def prev_relative_position(ignoring: nil) + def prev_relative_position prev_pos = nil if self.relative_position prev_pos = max_relative_position do |relation| - relation = relation.id_not_in(ignoring.id) if ignoring.present? relation.where('relative_position < ?', self.relative_position) end end @@ -210,12 +109,11 @@ module RelativePositioning prev_pos end - def next_relative_position(ignoring: nil) + def next_relative_position next_pos = nil if self.relative_position next_pos = min_relative_position do |relation| - relation = relation.id_not_in(ignoring.id) if ignoring.present? relation.where('relative_position > ?', self.relative_position) end end @@ -227,44 +125,24 @@ module RelativePositioning return move_after(before) unless after return move_before(after) unless before + # If there is no place to insert an item we need to create one by moving the item + # before this and all preceding items until there is a gap before, after = after, before if after.relative_position < before.relative_position - - pos_left = before.relative_position - pos_right = after.relative_position - - if pos_right - pos_left < MIN_GAP - # Not enough room! Make space by shifting all previous elements to the left - # if there is enough space, else to the right - gap = after.send(:find_next_gap_before) # rubocop:disable GitlabSecurity/PublicSend - - if gap.present? - after.move_sequence_before(next_gap: gap) - pos_left -= optimum_delta_for_gap(gap) - else - before.move_sequence_after - pos_right = after.reset.relative_position - end + if (after.relative_position - before.relative_position) < 2 + after.move_sequence_before + before.reset end - new_position = self.class.position_between(pos_left, pos_right) - - self.relative_position = new_position + self.relative_position = self.class.position_between(before.relative_position, after.relative_position) end def move_after(before = self) pos_before = before.relative_position - pos_after = before.next_relative_position(ignoring: self) - - if pos_before == MAX_POSITION || gap_too_small?(pos_after, pos_before) - gap = before.send(:find_next_gap_after) # rubocop:disable GitlabSecurity/PublicSend + pos_after = before.next_relative_position - if gap.nil? - before.move_sequence_before(true) - pos_before = before.reset.relative_position - else - before.move_sequence_after(next_gap: gap) - pos_after += optimum_delta_for_gap(gap) - end + if pos_after && (pos_after - pos_before) < 2 + before.move_sequence_after + pos_after = before.next_relative_position end self.relative_position = self.class.position_between(pos_before, pos_after) @@ -272,168 +150,80 @@ module RelativePositioning def move_before(after = self) pos_after = after.relative_position - pos_before = after.prev_relative_position(ignoring: self) + pos_before = after.prev_relative_position - if pos_after == MIN_POSITION || gap_too_small?(pos_before, pos_after) - gap = after.send(:find_next_gap_before) # rubocop:disable GitlabSecurity/PublicSend - - if gap.nil? - after.move_sequence_after(true) - pos_after = after.reset.relative_position - else - after.move_sequence_before(next_gap: gap) - pos_before -= optimum_delta_for_gap(gap) - end + if pos_before && (pos_after - pos_before) < 2 + after.move_sequence_before + pos_before = after.prev_relative_position end self.relative_position = self.class.position_between(pos_before, pos_after) end def move_to_end - max_pos = max_relative_position - - self.relative_position = max_pos.nil? ? START_POSITION : self.class.position_between(max_pos, MAX_POSITION) + self.relative_position = self.class.position_between(max_relative_position || START_POSITION, MAX_POSITION) end def move_to_start - min_pos = max_relative_position - - self.relative_position = min_pos.nil? ? START_POSITION : self.class.position_between(MIN_POSITION, min_pos) + self.relative_position = self.class.position_between(min_relative_position || START_POSITION, MIN_POSITION) end # Moves the sequence before the current item to the middle of the next gap - # For example, we have - # - # 5 . . . . . 11 12 13 14 [15] 16 . 17 - # ----------- - # - # This moves the sequence [11 12 13 14] to [8 9 10 11], so we have: - # - # 5 . . 8 9 10 11 . . . [15] 16 . 17 - # --------- - # - # Creating a gap to the left of the current item. We can understand this as - # dividing the 5 spaces between 5 and 11 into two smaller gaps of 2 and 3. - # - # If `include_self` is true, the current item will also be moved, creating a - # gap to the right of the current item: - # - # 5 . . 8 9 10 11 [14] . . . 16 . 17 - # -------------- - # - # As an optimization, the gap can be precalculated and passed to this method. - # - # @api private - # @raises NoSpaceLeft if the sequence cannot be moved - def move_sequence_before(include_self = false, next_gap: find_next_gap_before) - raise NoSpaceLeft unless next_gap.present? - + # For example, we have 5 11 12 13 14 15 and the current item is 15 + # This moves the sequence 11 12 13 14 to 8 9 10 11 + def move_sequence_before + next_gap = find_next_gap_before delta = optimum_delta_for_gap(next_gap) - move_sequence(next_gap[:start], relative_position, -delta, include_self) + move_sequence(next_gap[:start], relative_position, -delta) end # Moves the sequence after the current item to the middle of the next gap - # For example, we have: - # - # 8 . 10 [11] 12 13 14 15 . . . . . 21 - # ----------- - # - # This moves the sequence [12 13 14 15] to [15 16 17 18], so we have: - # - # 8 . 10 [11] . . . 15 16 17 18 . . 21 - # ----------- - # - # Creating a gap to the right of the current item. We can understand this as - # dividing the 5 spaces between 15 and 21 into two smaller gaps of 3 and 2. - # - # If `include_self` is true, the current item will also be moved, creating a - # gap to the left of the current item: - # - # 8 . 10 . . . [14] 15 16 17 18 . . 21 - # ---------------- - # - # As an optimization, the gap can be precalculated and passed to this method. - # - # @api private - # @raises NoSpaceLeft if the sequence cannot be moved - def move_sequence_after(include_self = false, next_gap: find_next_gap_after) - raise NoSpaceLeft unless next_gap.present? - + # For example, we have 11 12 13 14 15 21 and the current item is 11 + # This moves the sequence 12 13 14 15 to 15 16 17 18 + def move_sequence_after + next_gap = find_next_gap_after delta = optimum_delta_for_gap(next_gap) - move_sequence(relative_position, next_gap[:start], delta, include_self) + move_sequence(relative_position, next_gap[:start], delta) end private - def gap_too_small?(pos_a, pos_b) - return false unless pos_a && pos_b - - (pos_a - pos_b).abs < MIN_GAP - end - - # Find the first suitable gap to the left of the current position. - # - # Satisfies the relations: - # - gap[:start] <= relative_position - # - abs(gap[:start] - gap[:end]) >= MIN_GAP - # - MIN_POSITION <= gap[:start] <= MAX_POSITION - # - MIN_POSITION <= gap[:end] <= MAX_POSITION - # - # Supposing that the current item is 13, and we have a sequence of items: - # - # 1 . . . 5 . . . . 11 12 [13] 14 . . 17 - # ^---------^ - # - # Then we return: `{ start: 11, end: 5 }` - # - # Here start refers to the end of the gap closest to the current item. + # Supposing that we have a sequence of items: 1 5 11 12 13 and the current item is 13 + # This would return: `{ start: 11, end: 5 }` def find_next_gap_before items_with_next_pos = scoped_items .select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position DESC) AS next_pos') .where('relative_position <= ?', relative_position) .order(relative_position: :desc) - find_next_gap(items_with_next_pos, MIN_POSITION) + find_next_gap(items_with_next_pos).tap do |gap| + gap[:end] ||= MIN_POSITION + end end - # Find the first suitable gap to the right of the current position. - # - # Satisfies the relations: - # - gap[:start] >= relative_position - # - abs(gap[:start] - gap[:end]) >= MIN_GAP - # - MIN_POSITION <= gap[:start] <= MAX_POSITION - # - MIN_POSITION <= gap[:end] <= MAX_POSITION - # - # Supposing the current item is 13, and that we have a sequence of items: - # - # 9 . . . [13] 14 15 . . . . 20 . . . 24 - # ^---------^ - # - # Then we return: `{ start: 15, end: 20 }` - # - # Here start refers to the end of the gap closest to the current item. + # Supposing that we have a sequence of items: 13 14 15 20 24 and the current item is 13 + # This would return: `{ start: 15, end: 20 }` def find_next_gap_after items_with_next_pos = scoped_items .select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position ASC) AS next_pos') .where('relative_position >= ?', relative_position) .order(:relative_position) - find_next_gap(items_with_next_pos, MAX_POSITION) + find_next_gap(items_with_next_pos).tap do |gap| + gap[:end] ||= MAX_POSITION + end end - def find_next_gap(items_with_next_pos, end_is_nil) - gap = self.class - .from(items_with_next_pos, :items) - .where('next_pos IS NULL OR ABS(pos::bigint - next_pos::bigint) >= ?', MIN_GAP) - .limit(1) - .pluck(:pos, :next_pos) - .first + def find_next_gap(items_with_next_pos) + gap = self.class.from(items_with_next_pos, :items_with_next_pos) + .where('ABS(pos - next_pos) > 1 OR next_pos IS NULL') + .limit(1) + .pluck(:pos, :next_pos) + .first - return if gap.nil? || gap.first == end_is_nil - - { start: gap.first, end: gap.second || end_is_nil } + { start: gap[0], end: gap[1] } end def optimum_delta_for_gap(gap) @@ -442,10 +232,9 @@ module RelativePositioning [delta, IDEAL_DISTANCE].min end - def move_sequence(start_pos, end_pos, delta, include_self = false) - relation = include_self ? scoped_items : relative_siblings - - relation + def move_sequence(start_pos, end_pos, delta) + scoped_items + .where.not(id: self.id) .where('relative_position BETWEEN ? AND ?', start_pos, end_pos) .update_all("relative_position = relative_position + #{delta}") end @@ -466,10 +255,6 @@ module RelativePositioning .first&.last end - def relative_siblings(relation = scoped_items) - relation.id_not_in(id) - end - def scoped_items self.class.relative_positioning_query_base(self) end diff --git a/changelogs/unreleased/230477-design-pagination-2.yml b/changelogs/unreleased/230477-design-pagination-2.yml new file mode 100644 index 00000000000..8835ca59a2f --- /dev/null +++ b/changelogs/unreleased/230477-design-pagination-2.yml @@ -0,0 +1,5 @@ +--- +title: Update design mgmt navigation to use gl-button +merge_request: 39104 +author: +type: changed diff --git a/changelogs/unreleased/232934-sidekiq-versioning-flag.yml b/changelogs/unreleased/232934-sidekiq-versioning-flag.yml new file mode 100644 index 00000000000..6cef96e37e7 --- /dev/null +++ b/changelogs/unreleased/232934-sidekiq-versioning-flag.yml @@ -0,0 +1,5 @@ +--- +title: Provide versioning support to Sidekiq workers +merge_request: 39562 +author: +type: other diff --git a/changelogs/unreleased/238141-inclusion-of-event-it-in-api-return.yml b/changelogs/unreleased/238141-inclusion-of-event-it-in-api-return.yml new file mode 100644 index 00000000000..65860a52384 --- /dev/null +++ b/changelogs/unreleased/238141-inclusion-of-event-it-in-api-return.yml @@ -0,0 +1,5 @@ +--- +title: 'Expose ID in Event object returned from the public API.' +merge_request: 39669 +author: Killian Brackey @kbrackey +type: changed diff --git a/changelogs/unreleased/ajk-relative-positioning-improvements.yml b/changelogs/unreleased/ajk-relative-positioning-improvements.yml deleted file mode 100644 index c9de05a0403..00000000000 --- a/changelogs/unreleased/ajk-relative-positioning-improvements.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Performance and robustness improvements for relative positioning -merge_request: 37724 -author: -type: performance diff --git a/config/feature_flags/development/sidekiq_versioning.yml b/config/feature_flags/development/sidekiq_versioning.yml deleted file mode 100644 index 76b6e63adaa..00000000000 --- a/config/feature_flags/development/sidekiq_versioning.yml +++ /dev/null @@ -1,7 +0,0 @@ ---- -name: sidekiq_versioning -introduced_by_url: -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/232934 -group: group::fulfillment -type: development -default_enabled: false
\ No newline at end of file diff --git a/doc/api/events.md b/doc/api/events.md index 1ac1d72763a..3f4f11b9786 100644 --- a/doc/api/events.md +++ b/doc/api/events.md @@ -80,6 +80,7 @@ Example response: ```json [ { + "id": 1, "title":null, "project_id":1, "action_name":"opened", @@ -99,6 +100,7 @@ Example response: "author_username":"user3" }, { + "id": 2, "title":null, "project_id":1, "action_name":"opened", @@ -152,6 +154,7 @@ Example response: ```json [ { + "id": 3, "title": null, "project_id": 15, "action_name": "closed", @@ -170,6 +173,7 @@ Example response: "author_username": "root" }, { + "id": 4, "title": null, "project_id": 15, "action_name": "pushed", @@ -197,6 +201,7 @@ Example response: "target_title": null }, { + "id": 5, "title": null, "project_id": 15, "action_name": "closed", @@ -215,6 +220,7 @@ Example response: "author_username": "root" }, { + "id": 7, "title": null, "project_id": 15, "action_name": "commented on", @@ -286,6 +292,7 @@ Example response: ```json [ { + "id": 8 "title":null, "project_id":1, "action_name":"opened", @@ -306,6 +313,7 @@ Example response: "author_username":"user3" }, { + "id": 9, "title":null, "project_id":1, "action_name":"opened", @@ -326,6 +334,7 @@ Example response: "author_username":"ted" }, { + "id": 10, "title": null, "project_id": 1, "action_name": "commented on", diff --git a/doc/development/fe_guide/graphql.md b/doc/development/fe_guide/graphql.md index 51d0e33f67d..f5e16d377f1 100644 --- a/doc/development/fe_guide/graphql.md +++ b/doc/development/fe_guide/graphql.md @@ -466,6 +466,28 @@ fetchNextPage() { Please note we don't have to save `pageInfo` one more time; `fetchMore` triggers a query `result` hook as well. +### Managing performance + +The Apollo client will batch queries by default. This means that if you have 3 queries defined, +Apollo will group them into one request, send the single request off to the server and only +respond once all 3 queries have completed. + +If you need to have queries sent as individual requests, additional context can be provided +to tell Apollo to do this. + +```javascript +export default { + apollo: { + user: { + query: QUERY_IMPORT, + context: { + isSingleRequest: true, + } + } + }, +}; +``` + ### Testing #### Mocking response as component data diff --git a/lib/api/entities/event.rb b/lib/api/entities/event.rb index 8fd0bac13f4..f750d728e03 100644 --- a/lib/api/entities/event.rb +++ b/lib/api/entities/event.rb @@ -3,6 +3,7 @@ module API module Entities class Event < Grape::Entity + expose :id expose :project_id, :action_name expose :target_id, :target_iid, :target_type, :author_id expose :target_title diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index e7df9fd27f0..859b53b9887 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -22,7 +22,6 @@ module Gitlab # https://www.postgresql.org/docs/9.2/static/datatype-numeric.html MAX_INT_VALUE = 2147483647 - MIN_INT_VALUE = -2147483648 # The max value between MySQL's TIMESTAMP and PostgreSQL's timestampz: # https://www.postgresql.org/docs/9.1/static/datatype-datetime.html diff --git a/lib/gitlab/metrics/dashboard/stages/track_panel_type.rb b/lib/gitlab/metrics/dashboard/stages/track_panel_type.rb index 6e4ce770fa3..71da779d16c 100644 --- a/lib/gitlab/metrics/dashboard/stages/track_panel_type.rb +++ b/lib/gitlab/metrics/dashboard/stages/track_panel_type.rb @@ -18,7 +18,7 @@ module Gitlab def track_panel_type(panel) panel_type = panel[:type] - Gitlab::Tracking.event('MetricsDashboard::Chart', 'chart_rendered', label: 'Chart Type', value: panel_type) + Gitlab::Tracking.event('MetricsDashboard::Chart', 'chart_rendered', label: panel_type) end end end diff --git a/lib/gitlab/sidekiq_versioning/middleware.rb b/lib/gitlab/sidekiq_versioning/middleware.rb index e2d692c619b..2ffee617376 100644 --- a/lib/gitlab/sidekiq_versioning/middleware.rb +++ b/lib/gitlab/sidekiq_versioning/middleware.rb @@ -4,7 +4,7 @@ module Gitlab module SidekiqVersioning class Middleware def call(worker, job, queue) - worker.job_version = job['version'] if worker.is_a?(ApplicationWorker) && Feature.enabled?(:sidekiq_versioning) + worker.job_version = job['version'] if worker.is_a?(ApplicationWorker) yield end diff --git a/package.json b/package.json index 29d611c341d..15aba484c7b 100644 --- a/package.json +++ b/package.json @@ -43,7 +43,7 @@ "@babel/preset-env": "^7.10.1", "@gitlab/at.js": "1.5.5", "@gitlab/svgs": "1.158.0", - "@gitlab/ui": "20.1.1", + "@gitlab/ui": "20.3.1", "@gitlab/visual-review-tools": "1.6.1", "@rails/actioncable": "^6.0.3-1", "@sentry/browser": "^5.10.2", diff --git a/qa/qa/specs/features/api/3_create/gitaly/distributed_reads_spec.rb b/qa/qa/specs/features/api/3_create/gitaly/distributed_reads_spec.rb index 6bbfab653a6..6292ca821ca 100644 --- a/qa/qa/specs/features/api/3_create/gitaly/distributed_reads_spec.rb +++ b/qa/qa/specs/features/api/3_create/gitaly/distributed_reads_spec.rb @@ -7,7 +7,7 @@ module QA context 'Gitaly' do # Issue to track removal of feature flag: https://gitlab.com/gitlab-org/quality/team-tasks/-/issues/602 describe 'Distributed reads', :orchestrated, :gitaly_cluster, :skip_live_env, :requires_admin do - let(:number_of_reads) { 100 } + let(:number_of_reads_per_loop) { 9 } let(:praefect_manager) { Service::PraefectManager.new } let(:project) do Resource::Project.fabricate! do |project| @@ -28,16 +28,14 @@ module QA it 'reads from each node' do pre_read_data = praefect_manager.query_read_distribution - QA::Runtime::Logger.info('Fetching commits from the repository') - Parallel.each((1..number_of_reads)) { project.commits } - - praefect_manager.wait_for_read_count_change(pre_read_data) + wait_for_reads_to_increase(project, number_of_reads_per_loop, pre_read_data) aggregate_failures "each gitaly node" do praefect_manager.query_read_distribution.each_with_index do |data, index| - expect(data[:value]) - .to be > praefect_manager.value_for_node(pre_read_data, data[:node]), - "Read counts did not differ for node #{data[:node]}" + pre_read_count = praefect_manager.value_for_node(pre_read_data, data[:node]) + QA::Runtime::Logger.debug("Node: #{data[:node]}; before: #{pre_read_count}; now: #{data[:value]}") + expect(data[:value]).to be > pre_read_count, + "Read counts did not differ for node #{data[:node]}" end end end @@ -58,8 +56,7 @@ module QA it 'does not read from the unhealthy node' do pre_read_data = praefect_manager.query_read_distribution - QA::Runtime::Logger.info('Fetching commits from the repository') - Parallel.each((1..number_of_reads)) { project.commits } + read_from_project(project, number_of_reads_per_loop * 10) praefect_manager.wait_for_read_count_change(pre_read_data) @@ -72,6 +69,30 @@ module QA end end end + + def read_from_project(project, number_of_reads) + QA::Runtime::Logger.info('Reading from the repository') + Parallel.each((1..number_of_reads)) do + Git::Repository.perform do |repository| + repository.uri = project.repository_http_location.uri + repository.use_default_credentials + repository.clone + end + end + end + + def wait_for_reads_to_increase(project, number_of_reads, pre_read_data) + diff_found = pre_read_data + + Support::Waiter.wait_until(sleep_interval: 5, raise_on_failure: false) do + read_from_project(project, number_of_reads) + + praefect_manager.query_read_distribution.each_with_index do |data, index| + diff_found[index][:diff] = true if data[:value] > praefect_manager.value_for_node(pre_read_data, data[:node]) + end + diff_found.all? { |node| node.key?(:diff) && node[:diff] } + end + end end end end diff --git a/spec/features/boards/issue_ordering_spec.rb b/spec/features/boards/issue_ordering_spec.rb index 87d29eed68d..03a76d9d3fd 100644 --- a/spec/features/boards/issue_ordering_spec.rb +++ b/spec/features/boards/issue_ordering_spec.rb @@ -21,7 +21,7 @@ RSpec.describe 'Issue Boards', :js do end context 'un-ordered issues' do - let!(:issue4) { create(:labeled_issue, project: project, labels: [label], relative_position: nil) } + let!(:issue4) { create(:labeled_issue, project: project, labels: [label]) } before do visit project_board_path(project, board) diff --git a/spec/features/issues/user_sorts_issues_spec.rb b/spec/features/issues/user_sorts_issues_spec.rb index 91c6419b464..f0bb055c6f2 100644 --- a/spec/features/issues/user_sorts_issues_spec.rb +++ b/spec/features/issues/user_sorts_issues_spec.rb @@ -16,8 +16,6 @@ RSpec.describe "User sorts issues" do let_it_be(:later_due_milestone) { create(:milestone, project: project, due_date: '2013-12-12') } before do - stub_feature_flags(vue_issuables_list: false) - create_list(:award_emoji, 2, :upvote, awardable: issue1) create_list(:award_emoji, 2, :downvote, awardable: issue2) create(:award_emoji, :downvote, awardable: issue1) @@ -48,7 +46,7 @@ RSpec.describe "User sorts issues" do expect(find('.issues-filters a.is-active')).to have_content('Milestone') end - it "sorts by popularity" do + it 'sorts by popularity', :js do find('.filter-dropdown-container .dropdown').click page.within('ul.dropdown-menu.dropdown-menu-right li') do @@ -70,14 +68,14 @@ RSpec.describe "User sorts issues" do end end - it 'sorts by newest' do + it 'sorts by newest', :js do visit project_issues_path(project, sort: sort_value_created_date) expect(first_issue).to include('foo') expect(last_issue).to include('baz') end - it 'sorts by most recently updated' do + it 'sorts by most recently updated', :js do issue3.updated_at = Time.now + 100 issue3.save visit project_issues_path(project, sort: sort_value_recently_updated) @@ -85,7 +83,7 @@ RSpec.describe "User sorts issues" do expect(first_issue).to include('baz') end - describe 'sorting by due date' do + describe 'sorting by due date', :js do before do issue1.update(due_date: 1.day.from_now) issue2.update(due_date: 6.days.from_now) @@ -122,7 +120,7 @@ RSpec.describe "User sorts issues" do end end - describe 'filtering by due date' do + describe 'filtering by due date', :js do before do issue1.update(due_date: 1.day.from_now) issue2.update(due_date: 6.days.from_now) @@ -205,7 +203,7 @@ RSpec.describe "User sorts issues" do end end - describe 'sorting by milestone' do + describe 'sorting by milestone', :js do before do issue1.milestone = newer_due_milestone issue1.save @@ -221,7 +219,7 @@ RSpec.describe "User sorts issues" do end end - describe 'combine filter and sort' do + describe 'combine filter and sort', :js do let(:user2) { create(:user) } before do diff --git a/spec/frontend/design_management/components/toolbar/__snapshots__/pagination_spec.js.snap b/spec/frontend/design_management/components/toolbar/__snapshots__/design_navigation_spec.js.snap index 83b631cc86a..a7d6145285c 100644 --- a/spec/frontend/design_management/components/toolbar/__snapshots__/pagination_spec.js.snap +++ b/spec/frontend/design_management/components/toolbar/__snapshots__/design_navigation_spec.js.snap @@ -2,28 +2,34 @@ exports[`Design management pagination component hides components when designs are empty 1`] = `<!---->`; -exports[`Design management pagination component renders pagination buttons 1`] = ` +exports[`Design management pagination component renders navigation buttons 1`] = ` <div class="d-flex align-items-center" > 0 of 2 - <div - class="btn-group gl-ml-3" + <gl-button-group-stub + class="ml-3 mr-3" > - <pagination-button-stub + <gl-button-stub + category="primary" class="js-previous-design" - iconname="angle-left" + disabled="true" + icon="angle-left" + size="medium" title="Go to previous design" + variant="default" /> - <pagination-button-stub + <gl-button-stub + category="primary" class="js-next-design" - design="[object Object]" - iconname="angle-right" + icon="angle-right" + size="medium" title="Go to next design" + variant="default" /> - </div> + </gl-button-group-stub> </div> `; diff --git a/spec/frontend/design_management/components/toolbar/__snapshots__/index_spec.js.snap b/spec/frontend/design_management/components/toolbar/__snapshots__/index_spec.js.snap index 1900c64cd47..b286a74ebb8 100644 --- a/spec/frontend/design_management/components/toolbar/__snapshots__/index_spec.js.snap +++ b/spec/frontend/design_management/components/toolbar/__snapshots__/index_spec.js.snap @@ -35,31 +35,27 @@ exports[`Design management toolbar component renders design and updated data 1`] </div> </div> - <div - class="gl-display-flex gl-align-items-center" - > - <pagination-stub - class="gl-mr-3 gl-flex-shrink-0" - id="1" - /> - - <gl-button-stub - category="primary" - href="/-/designs/306/7f747adcd4693afadbe968d7ba7d983349b9012d" - icon="download" - size="medium" - variant="default" - /> - - <delete-button-stub - buttoncategory="secondary" - buttonclass="" - buttonicon="archive" - buttonsize="medium" - buttonvariant="warning" - class="gl-ml-3" - hasselecteddesigns="true" - /> - </div> + <design-navigation-stub + class="ml-auto flex-shrink-0" + id="1" + /> + + <gl-button-stub + category="primary" + href="/-/designs/306/7f747adcd4693afadbe968d7ba7d983349b9012d" + icon="download" + size="medium" + variant="default" + /> + + <delete-button-stub + buttoncategory="secondary" + buttonclass="" + buttonicon="archive" + buttonsize="medium" + buttonvariant="warning" + class="gl-ml-3" + hasselecteddesigns="true" + /> </header> `; diff --git a/spec/frontend/design_management/components/toolbar/__snapshots__/pagination_button_spec.js.snap b/spec/frontend/design_management/components/toolbar/__snapshots__/pagination_button_spec.js.snap deleted file mode 100644 index 08662a04f15..00000000000 --- a/spec/frontend/design_management/components/toolbar/__snapshots__/pagination_button_spec.js.snap +++ /dev/null @@ -1,28 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`Design management pagination button component disables button when no design is passed 1`] = ` -<router-link-stub - aria-label="Test title" - class="btn btn-default disabled" - disabled="true" - to="[object Object]" -> - <icon-stub - name="angle-right" - size="16" - /> -</router-link-stub> -`; - -exports[`Design management pagination button component renders router-link 1`] = ` -<router-link-stub - aria-label="Test title" - class="btn btn-default" - to="[object Object]" -> - <icon-stub - name="angle-right" - size="16" - /> -</router-link-stub> -`; diff --git a/spec/frontend/design_management/components/toolbar/pagination_spec.js b/spec/frontend/design_management/components/toolbar/design_navigation_spec.js index db5a36dadf6..1c6588a9628 100644 --- a/spec/frontend/design_management/components/toolbar/pagination_spec.js +++ b/spec/frontend/design_management/components/toolbar/design_navigation_spec.js @@ -1,7 +1,7 @@ /* global Mousetrap */ import 'mousetrap'; import { shallowMount } from '@vue/test-utils'; -import Pagination from '~/design_management/components/toolbar/pagination.vue'; +import DesignNavigation from '~/design_management/components/toolbar/design_navigation.vue'; import { DESIGN_ROUTE_NAME } from '~/design_management/router/constants'; const push = jest.fn(); @@ -18,7 +18,7 @@ describe('Design management pagination component', () => { let wrapper; function createComponent() { - wrapper = shallowMount(Pagination, { + wrapper = shallowMount(DesignNavigation, { propsData: { id: '2', }, @@ -41,7 +41,7 @@ describe('Design management pagination component', () => { expect(wrapper.element).toMatchSnapshot(); }); - it('renders pagination buttons', () => { + it('renders navigation buttons', () => { wrapper.setData({ designs: [{ id: '1' }, { id: '2' }], }); diff --git a/spec/frontend/design_management/components/toolbar/pagination_button_spec.js b/spec/frontend/design_management/components/toolbar/pagination_button_spec.js deleted file mode 100644 index b7df201795b..00000000000 --- a/spec/frontend/design_management/components/toolbar/pagination_button_spec.js +++ /dev/null @@ -1,61 +0,0 @@ -import { createLocalVue, shallowMount } from '@vue/test-utils'; -import VueRouter from 'vue-router'; -import PaginationButton from '~/design_management/components/toolbar/pagination_button.vue'; -import { DESIGN_ROUTE_NAME } from '~/design_management/router/constants'; - -const localVue = createLocalVue(); -localVue.use(VueRouter); -const router = new VueRouter(); - -describe('Design management pagination button component', () => { - let wrapper; - - function createComponent(design = null) { - wrapper = shallowMount(PaginationButton, { - localVue, - router, - propsData: { - design, - title: 'Test title', - iconName: 'angle-right', - }, - stubs: ['router-link'], - }); - } - - afterEach(() => { - wrapper.destroy(); - }); - - it('disables button when no design is passed', () => { - createComponent(); - - expect(wrapper.element).toMatchSnapshot(); - }); - - it('renders router-link', () => { - createComponent({ id: '2' }); - - expect(wrapper.element).toMatchSnapshot(); - }); - - describe('designLink', () => { - it('returns empty link when design is null', () => { - createComponent(); - - expect(wrapper.vm.designLink).toEqual({}); - }); - - it('returns design link', () => { - createComponent({ id: '2', filename: 'test' }); - - wrapper.vm.$router.replace('/root/test-project/issues/1/designs/test?version=1'); - - expect(wrapper.vm.designLink).toEqual({ - name: DESIGN_ROUTE_NAME, - params: { id: 'test' }, - query: { version: '1' }, - }); - }); - }); -}); diff --git a/spec/lib/gitlab/metrics/dashboard/stages/track_panel_type_spec.rb b/spec/lib/gitlab/metrics/dashboard/stages/track_panel_type_spec.rb index 53f1d9a74db..d9987b67127 100644 --- a/spec/lib/gitlab/metrics/dashboard/stages/track_panel_type_spec.rb +++ b/spec/lib/gitlab/metrics/dashboard/stages/track_panel_type_spec.rb @@ -14,10 +14,14 @@ RSpec.describe Gitlab::Metrics::Dashboard::Stages::TrackPanelType do let(:dashboard) { load_sample_dashboard.deep_symbolize_keys } it 'creates tracking event' do - expect(Gitlab::Tracking).to receive(:event).with('MetricsDashboard::Chart', 'chart_rendered', - { label: 'Chart Type', value: 'area-chart' }).at_least(:once) + stub_application_setting(snowplow_enabled: true, snowplow_collector_hostname: 'localhost') + allow(Gitlab::Tracking).to receive(:event).and_call_original subject.transform! + + expect(Gitlab::Tracking).to have_received(:event) + .with('MetricsDashboard::Chart', 'chart_rendered', { label: 'area-chart' }) + .at_least(:once) end end end diff --git a/spec/lib/gitlab/sidekiq_versioning/middleware_spec.rb b/spec/lib/gitlab/sidekiq_versioning/middleware_spec.rb index dc4d1971a98..b372f16de5e 100644 --- a/spec/lib/gitlab/sidekiq_versioning/middleware_spec.rb +++ b/spec/lib/gitlab/sidekiq_versioning/middleware_spec.rb @@ -44,17 +44,5 @@ RSpec.describe Gitlab::SidekiqVersioning::Middleware do expect { call! }.not_to raise_error end end - - context 'when sidekiq_versioning is disabled' do - before do - stub_feature_flags(sidekiq_versioning: false) - end - - it 'does not set job_version' do - expect(worker).not_to receive(:job_version=) - - call! - end - end end end diff --git a/spec/models/analytics/cycle_analytics/project_stage_spec.rb b/spec/models/analytics/cycle_analytics/project_stage_spec.rb index 4675f037957..2e024011553 100644 --- a/spec/models/analytics/cycle_analytics/project_stage_spec.rb +++ b/spec/models/analytics/cycle_analytics/project_stage_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Analytics::CycleAnalytics::ProjectStage do context 'relative positioning' do it_behaves_like 'a class that supports relative positioning' do - let_it_be(:project) { create(:project) } + let(:project) { build(:project) } let(:factory) { :cycle_analytics_project_stage } let(:default_params) { { project: project } } end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 59634524e74..586ef9da98e 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -6,7 +6,6 @@ RSpec.describe Issue do include ExternalAuthorizationServiceHelpers let_it_be(:user) { create(:user) } - let_it_be(:reusable_project) { create(:project) } describe "Associations" do it { is_expected.to belong_to(:milestone) } @@ -146,13 +145,13 @@ RSpec.describe Issue do end describe '#order_by_position_and_priority' do - let(:project) { reusable_project } + let(:project) { create :project } let(:p1) { create(:label, title: 'P1', project: project, priority: 1) } let(:p2) { create(:label, title: 'P2', project: project, priority: 2) } let!(:issue1) { create(:labeled_issue, project: project, labels: [p1]) } let!(:issue2) { create(:labeled_issue, project: project, labels: [p2]) } - let!(:issue3) { create(:issue, project: project, relative_position: -200) } - let!(:issue4) { create(:issue, project: project, relative_position: -100) } + let!(:issue3) { create(:issue, project: project, relative_position: 100) } + let!(:issue4) { create(:issue, project: project, relative_position: 200) } it 'returns ordered list' do expect(project.issues.order_by_position_and_priority) @@ -161,10 +160,10 @@ RSpec.describe Issue do end describe '#sort' do - let(:project) { reusable_project } + let(:project) { create(:project) } context "by relative_position" do - let!(:issue) { create(:issue, project: project, relative_position: nil) } + let!(:issue) { create(:issue, project: project) } let!(:issue2) { create(:issue, project: project, relative_position: 2) } let!(:issue3) { create(:issue, project: project, relative_position: 1) } @@ -1028,7 +1027,7 @@ RSpec.describe Issue do context "relative positioning" do it_behaves_like "a class that supports relative positioning" do - let_it_be(:project) { create(:project) } + let(:project) { create(:project) } let(:factory) { :issue } let(:default_params) { { project: project } } end diff --git a/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb b/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb index 6bfbbf9b076..aea9c25d104 100644 --- a/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb +++ b/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb @@ -58,11 +58,13 @@ RSpec.describe Metrics::Dashboard::CustomDashboardService, :use_clean_rails_memo end it 'tracks panel type' do - expect(::Gitlab::Tracking).to receive(:event).with( - 'MetricsDashboard::Chart', 'chart_rendered', { label: 'Chart Type', value: 'area-chart' } - ).at_least(:once) + allow(::Gitlab::Tracking).to receive(:event).and_call_original described_class.new(*service_params).get_dashboard + + expect(::Gitlab::Tracking).to have_received(:event) + .with('MetricsDashboard::Chart', 'chart_rendered', { label: 'area-chart' }) + .at_least(:once) end context 'and the dashboard is then deleted' do diff --git a/spec/support/shared_examples/models/relative_positioning_shared_examples.rb b/spec/support/shared_examples/models/relative_positioning_shared_examples.rb index d09eb1fa14d..ab1467145a0 100644 --- a/spec/support/shared_examples/models/relative_positioning_shared_examples.rb +++ b/spec/support/shared_examples/models/relative_positioning_shared_examples.rb @@ -25,6 +25,7 @@ RSpec.shared_examples 'a class that supports relative positioning' do items = [item1, item2, item3] described_class.move_nulls_to_end(items) + items.map(&:reload) expect(items.sort_by(&:relative_position)).to eq(items) expect(item1.relative_position).to be(1000) @@ -34,57 +35,22 @@ RSpec.shared_examples 'a class that supports relative positioning' do expect(item3.next_relative_position).to be_nil end - it 'preserves relative position' do - item1.update!(relative_position: nil) - item2.update!(relative_position: nil) - - described_class.move_nulls_to_end([item1, item2]) - - expect(item1.relative_position).to be < item2.relative_position - end - it 'moves the item near the start position when there are no existing positions' do item1.update!(relative_position: nil) described_class.move_nulls_to_end([item1]) - expect(item1.reset.relative_position).to eq(described_class::START_POSITION + described_class::IDEAL_DISTANCE) + item1.reload + + expect(item1.relative_position).to eq(described_class::START_POSITION + described_class::IDEAL_DISTANCE) end it 'does not perform any moves if all items have their relative_position set' do item1.update!(relative_position: 1) - expect do - described_class.move_nulls_to_end([item1]) - end.not_to change { item1.reset.relative_position } - end - - it 'manages to move nulls to the end even if there is a sequence at the end' do - bunch = create_items_with_positions(run_at_end) - item1.update!(relative_position: nil) - described_class.move_nulls_to_end([item1]) + item1.reload - items = [*bunch, item1] - items.each(&:reset) - - expect(items.map(&:relative_position)).to all(be_valid_position) - expect(items.sort_by(&:relative_position)).to eq(items) - end - - it 'does not have an N+1 issue' do - create_items_with_positions(10..12) - - a, b, c, d, e, f = create_items_with_positions([nil, nil, nil, nil, nil, nil]) - - baseline = ActiveRecord::QueryRecorder.new do - described_class.move_nulls_to_end([a, e]) - end - - expect { described_class.move_nulls_to_end([b, c, d]) } - .not_to exceed_query_limit(baseline) - - expect { described_class.move_nulls_to_end([f]) } - .not_to exceed_query_limit(baseline.count) + expect(item1.relative_position).to be(1) end end @@ -112,19 +78,11 @@ RSpec.shared_examples 'a class that supports relative positioning' do item1.update!(relative_position: nil) described_class.move_nulls_to_start([item1]) + item1.reload expect(item1.relative_position).to eq(described_class::START_POSITION - described_class::IDEAL_DISTANCE) end - it 'preserves relative position' do - item1.update!(relative_position: nil) - item2.update!(relative_position: nil) - - described_class.move_nulls_to_end([item1, item2]) - - expect(item1.relative_position).to be < item2.relative_position - end - it 'does not perform any moves if all items have their relative_position set' do item1.update!(relative_position: 1) @@ -143,8 +101,8 @@ RSpec.shared_examples 'a class that supports relative positioning' do describe '#prev_relative_position' do it 'returns previous position if there is an item above' do - item1.update!(relative_position: 5) - item2.update!(relative_position: 15) + item1.update(relative_position: 5) + item2.update(relative_position: 15) expect(item2.prev_relative_position).to eq item1.relative_position end @@ -156,8 +114,8 @@ RSpec.shared_examples 'a class that supports relative positioning' do describe '#next_relative_position' do it 'returns next position if there is an item below' do - item1.update!(relative_position: 5) - item2.update!(relative_position: 15) + item1.update(relative_position: 5) + item2.update(relative_position: 15) expect(item1.next_relative_position).to eq item2.relative_position end @@ -167,172 +125,9 @@ RSpec.shared_examples 'a class that supports relative positioning' do end end - describe '#find_next_gap_before' do - context 'there is no gap' do - let(:items) { create_items_with_positions(run_at_start) } - - it 'returns nil' do - items.each do |item| - expect(item.send(:find_next_gap_before)).to be_nil - end - end - end - - context 'there is a sequence ending at MAX_POSITION' do - let(:items) { create_items_with_positions(run_at_end) } - - let(:gaps) do - items.map { |item| item.send(:find_next_gap_before) } - end - - it 'can find the gap at the start for any item in the sequence' do - gap = { start: items.first.relative_position, end: RelativePositioning::MIN_POSITION } - - expect(gaps).to all(eq(gap)) - end - - it 'respects lower bounds' do - gap = { start: items.first.relative_position, end: 10 } - new_item.update!(relative_position: 10) - - expect(gaps).to all(eq(gap)) - end - end - - specify do - item1.update!(relative_position: 5) - - (0..10).each do |pos| - item2.update!(relative_position: pos) - - gap = item2.send(:find_next_gap_before) - - expect(gap[:start]).to be <= item2.relative_position - expect((gap[:end] - gap[:start]).abs).to be >= RelativePositioning::MIN_GAP - expect(gap[:start]).to be_valid_position - expect(gap[:end]).to be_valid_position - end - end - - it 'deals with there not being any items to the left' do - create_items_with_positions([1, 2, 3]) - new_item.update!(relative_position: 0) - - expect(new_item.send(:find_next_gap_before)).to eq(start: 0, end: RelativePositioning::MIN_POSITION) - end - - it 'finds the next gap to the left, skipping adjacent values' do - create_items_with_positions([1, 9, 10]) - new_item.update!(relative_position: 11) - - expect(new_item.send(:find_next_gap_before)).to eq(start: 9, end: 1) - end - - it 'finds the next gap to the left' do - create_items_with_positions([2, 10]) - - new_item.update!(relative_position: 15) - expect(new_item.send(:find_next_gap_before)).to eq(start: 15, end: 10) - - new_item.update!(relative_position: 11) - expect(new_item.send(:find_next_gap_before)).to eq(start: 10, end: 2) - - new_item.update!(relative_position: 9) - expect(new_item.send(:find_next_gap_before)).to eq(start: 9, end: 2) - - new_item.update!(relative_position: 5) - expect(new_item.send(:find_next_gap_before)).to eq(start: 5, end: 2) - end - end - - describe '#find_next_gap_after' do - context 'there is no gap' do - let(:items) { create_items_with_positions(run_at_end) } - - it 'returns nil' do - items.each do |item| - expect(item.send(:find_next_gap_after)).to be_nil - end - end - end - - context 'there is a sequence starting at MIN_POSITION' do - let(:items) { create_items_with_positions(run_at_start) } - - let(:gaps) do - items.map { |item| item.send(:find_next_gap_after) } - end - - it 'can find the gap at the end for any item in the sequence' do - gap = { start: items.last.relative_position, end: RelativePositioning::MAX_POSITION } - - expect(gaps).to all(eq(gap)) - end - - it 'respects upper bounds' do - gap = { start: items.last.relative_position, end: 10 } - new_item.update!(relative_position: 10) - - expect(gaps).to all(eq(gap)) - end - end - - specify do - item1.update!(relative_position: 5) - - (0..10).each do |pos| - item2.update!(relative_position: pos) - - gap = item2.send(:find_next_gap_after) - - expect(gap[:start]).to be >= item2.relative_position - expect((gap[:end] - gap[:start]).abs).to be >= RelativePositioning::MIN_GAP - expect(gap[:start]).to be_valid_position - expect(gap[:end]).to be_valid_position - end - end - - it 'deals with there not being any items to the right' do - create_items_with_positions([1, 2, 3]) - new_item.update!(relative_position: 5) - - expect(new_item.send(:find_next_gap_after)).to eq(start: 5, end: RelativePositioning::MAX_POSITION) - end - - it 'finds the next gap to the right, skipping adjacent values' do - create_items_with_positions([1, 2, 10]) - new_item.update!(relative_position: 0) - - expect(new_item.send(:find_next_gap_after)).to eq(start: 2, end: 10) - end - - it 'finds the next gap to the right' do - create_items_with_positions([2, 10]) - - new_item.update!(relative_position: 0) - expect(new_item.send(:find_next_gap_after)).to eq(start: 0, end: 2) - - new_item.update!(relative_position: 1) - expect(new_item.send(:find_next_gap_after)).to eq(start: 2, end: 10) - - new_item.update!(relative_position: 3) - expect(new_item.send(:find_next_gap_after)).to eq(start: 3, end: 10) - - new_item.update!(relative_position: 5) - expect(new_item.send(:find_next_gap_after)).to eq(start: 5, end: 10) - end - end - describe '#move_before' do - let(:item3) { create(factory, default_params) } - it 'moves item before' do - [item2, item1].each do |item| - item.move_to_end - item.save! - end - - expect(item1.relative_position).to be > item2.relative_position + [item2, item1].each(&:move_to_end) item1.move_before(item2) @@ -340,10 +135,12 @@ RSpec.shared_examples 'a class that supports relative positioning' do end context 'when there is no space' do + let(:item3) { create(factory, default_params) } + before do - item1.update!(relative_position: 1000) - item2.update!(relative_position: 1001) - item3.update!(relative_position: 1002) + item1.update(relative_position: 1000) + item2.update(relative_position: 1001) + item3.update(relative_position: 1002) end it 'moves items correctly' do @@ -352,73 +149,6 @@ RSpec.shared_examples 'a class that supports relative positioning' do expect(item3.relative_position).to be_between(item1.reload.relative_position, item2.reload.relative_position).exclusive end end - - it 'can move the item before an item at the start' do - item1.update!(relative_position: RelativePositioning::START_POSITION) - - new_item.move_before(item1) - - expect(new_item.relative_position).to be_valid_position - expect(new_item.relative_position).to be < item1.reload.relative_position - end - - it 'can move the item before an item at MIN_POSITION' do - item1.update!(relative_position: RelativePositioning::MIN_POSITION) - - new_item.move_before(item1) - - expect(new_item.relative_position).to be >= RelativePositioning::MIN_POSITION - expect(new_item.relative_position).to be < item1.reload.relative_position - end - - it 'can move the item before an item bunched up at MIN_POSITION' do - item1, item2, item3 = create_items_with_positions(run_at_start) - - new_item.move_before(item3) - new_item.save! - - items = [item1, item2, new_item, item3] - - items.each do |item| - expect(item.reset.relative_position).to be_valid_position - end - - expect(items.sort_by(&:relative_position)).to eq(items) - end - - context 'leap-frogging to the left' do - before do - start = RelativePositioning::START_POSITION - item1.update!(relative_position: start - RelativePositioning::IDEAL_DISTANCE * 0) - item2.update!(relative_position: start - RelativePositioning::IDEAL_DISTANCE * 1) - item3.update!(relative_position: start - RelativePositioning::IDEAL_DISTANCE * 2) - end - - let(:item3) { create(factory, default_params) } - - def leap_frog(steps) - a = item1 - b = item2 - - steps.times do |i| - a.move_before(b) - a.save! - a, b = b, a - end - end - - it 'can leap-frog STEPS - 1 times before needing to rebalance' do - # This is less efficient than going right, due to the flooring of - # integer division - expect { leap_frog(RelativePositioning::STEPS - 1) } - .not_to change { item3.reload.relative_position } - end - - it 'rebalances after leap-frogging STEPS times' do - expect { leap_frog(RelativePositioning::STEPS) } - .to change { item3.reload.relative_position } - end - end end describe '#move_after' do @@ -434,17 +164,9 @@ RSpec.shared_examples 'a class that supports relative positioning' do let(:item3) { create(factory, default_params) } before do - item1.update!(relative_position: 1000) - item2.update!(relative_position: 1001) - item3.update!(relative_position: 1002) - end - - it 'can move the item after an item at MAX_POSITION' do - item1.update!(relative_position: RelativePositioning::MAX_POSITION) - - new_item.move_after(item1) - expect(new_item.relative_position).to be_valid_position - expect(new_item.relative_position).to be > item1.reset.relative_position + item1.update(relative_position: 1000) + item2.update(relative_position: 1001) + item3.update(relative_position: 1002) end it 'moves items correctly' do @@ -453,53 +175,6 @@ RSpec.shared_examples 'a class that supports relative positioning' do expect(item1.relative_position).to be_between(item2.reload.relative_position, item3.reload.relative_position).exclusive end end - - it 'can move the item after an item bunched up at MAX_POSITION' do - item1, item2, item3 = create_items_with_positions(run_at_end) - - new_item.move_after(item1) - new_item.save! - - items = [item1, new_item, item2, item3] - - items.each do |item| - expect(item.reset.relative_position).to be_valid_position - end - - expect(items.sort_by(&:relative_position)).to eq(items) - end - - context 'leap-frogging' do - before do - start = RelativePositioning::START_POSITION - item1.update!(relative_position: start + RelativePositioning::IDEAL_DISTANCE * 0) - item2.update!(relative_position: start + RelativePositioning::IDEAL_DISTANCE * 1) - item3.update!(relative_position: start + RelativePositioning::IDEAL_DISTANCE * 2) - end - - let(:item3) { create(factory, default_params) } - - def leap_frog(steps) - a = item1 - b = item2 - - steps.times do |i| - a.move_after(b) - a.save! - a, b = b, a - end - end - - it 'can leap-frog STEPS times before needing to rebalance' do - expect { leap_frog(RelativePositioning::STEPS) } - .not_to change { item3.reload.relative_position } - end - - it 'rebalances after leap-frogging STEPS+1 times' do - expect { leap_frog(RelativePositioning::STEPS + 1) } - .to change { item3.reload.relative_position } - end - end end describe '#move_to_end' do @@ -518,17 +193,8 @@ RSpec.shared_examples 'a class that supports relative positioning' do describe '#move_between' do before do - [item1, item2].each do |item| - item.move_to_end && item.save! - end - end - - shared_examples 'moves item between' do - it 'moves the middle item to between left and right' do - expect do - middle.move_between(left, right) - middle.save! - end.to change { between_exclusive?(left, middle, right) }.from(false).to(true) + [item1, item2].each do |item1| + item1.move_to_end && item1.save end end @@ -552,26 +218,26 @@ RSpec.shared_examples 'a class that supports relative positioning' do end it 'positions items even when after and before positions are the same' do - item2.update! relative_position: item1.relative_position + item2.update relative_position: item1.relative_position new_item.move_between(item1, item2) - [item1, item2].each(&:reset) expect(new_item.relative_position).to be > item1.relative_position expect(item1.relative_position).to be < item2.relative_position end - context 'the two items are next to each other' do - let(:left) { item1 } - let(:middle) { new_item } - let(:right) { create_item(relative_position: item1.relative_position + 1) } + it 'positions items between other two if distance is 1' do + item2.update relative_position: item1.relative_position + 1 - it_behaves_like 'moves item between' + new_item.move_between(item1, item2) + + expect(new_item.relative_position).to be > item1.relative_position + expect(item1.relative_position).to be < item2.relative_position end it 'positions item in the middle of other two if distance is big enough' do - item1.update! relative_position: 6000 - item2.update! relative_position: 10000 + item1.update relative_position: 6000 + item2.update relative_position: 10000 new_item.move_between(item1, item2) @@ -579,8 +245,7 @@ RSpec.shared_examples 'a class that supports relative positioning' do end it 'positions item closer to the middle if we are at the very top' do - item1.update!(relative_position: 6001) - item2.update!(relative_position: 6000) + item2.update relative_position: 6000 new_item.move_between(nil, item2) @@ -588,53 +253,51 @@ RSpec.shared_examples 'a class that supports relative positioning' do end it 'positions item closer to the middle if we are at the very bottom' do - new_item.update!(relative_position: 1) - item1.update!(relative_position: 6000) - item2.update!(relative_position: 5999) + new_item.update relative_position: 1 + item1.update relative_position: 6000 + item2.destroy new_item.move_between(item1, nil) expect(new_item.relative_position).to eq(6000 + RelativePositioning::IDEAL_DISTANCE) end - it 'positions item in the middle of other two' do - item1.update! relative_position: 100 - item2.update! relative_position: 400 + it 'positions item in the middle of other two if distance is not big enough' do + item1.update relative_position: 100 + item2.update relative_position: 400 new_item.move_between(item1, item2) expect(new_item.relative_position).to eq(250) end - context 'there is no space' do - let(:middle) { new_item } - let(:left) { create_item(relative_position: 100) } - let(:right) { create_item(relative_position: 101) } + it 'positions item in the middle of other two is there is no place' do + item1.update relative_position: 100 + item2.update relative_position: 101 - it_behaves_like 'moves item between' - end + new_item.move_between(item1, item2) - context 'there is a bunch of items' do - let(:items) { create_items_with_positions(100..104) } - let(:left) { items[1] } - let(:middle) { items[3] } - let(:right) { items[2] } + expect(new_item.relative_position).to be_between(item1.relative_position, item2.relative_position).exclusive + end - it_behaves_like 'moves item between' + it 'uses rebalancing if there is no place' do + item1.update relative_position: 100 + item2.update relative_position: 101 + item3 = create_item(relative_position: 102) + new_item.update relative_position: 103 - it 'handles bunches correctly' do - middle.move_between(left, right) - middle.save! + new_item.move_between(item2, item3) + new_item.save! - expect(items.first.reset.relative_position).to be < middle.relative_position - end + expect(new_item.relative_position).to be_between(item2.relative_position, item3.relative_position).exclusive + expect(item1.reload.relative_position).not_to eq(100) end - it 'positions item right if we pass non-sequential parameters' do - item1.update! relative_position: 99 - item2.update! relative_position: 101 + it 'positions item right if we pass none-sequential parameters' do + item1.update relative_position: 99 + item2.update relative_position: 101 item3 = create_item(relative_position: 102) - new_item.update! relative_position: 103 + new_item.update relative_position: 103 new_item.move_between(item1, item3) new_item.save! @@ -666,12 +329,6 @@ RSpec.shared_examples 'a class that supports relative positioning' do expect(positions).to eq([90, 95, 96, 102]) end - it 'raises an error if there is no space' do - items = create_items_with_positions(run_at_start) - - expect { items.last.move_sequence_before }.to raise_error(RelativePositioning::NoSpaceLeft) - end - it 'finds a gap if there are unused positions' do items = create_items_with_positions([100, 101, 102]) @@ -679,8 +336,7 @@ RSpec.shared_examples 'a class that supports relative positioning' do items.last.save! positions = items.map { |item| item.reload.relative_position } - - expect(positions.last - positions.second).to be > RelativePositioning::MIN_GAP + expect(positions).to eq([50, 51, 102]) end end @@ -702,33 +358,7 @@ RSpec.shared_examples 'a class that supports relative positioning' do items.first.save! positions = items.map { |item| item.reload.relative_position } - expect(positions.second - positions.first).to be > RelativePositioning::MIN_GAP + expect(positions).to eq([100, 601, 602]) end - - it 'raises an error if there is no space' do - items = create_items_with_positions(run_at_end) - - expect { items.first.move_sequence_after }.to raise_error(RelativePositioning::NoSpaceLeft) - end - end - - def be_valid_position - be_between(RelativePositioning::MIN_POSITION, RelativePositioning::MAX_POSITION) - end - - def between_exclusive?(left, middle, right) - a, b, c = [left, middle, right].map { |item| item.reset.relative_position } - return false if a.nil? || b.nil? - return a < b if c.nil? - - a < b && b < c - end - - def run_at_end(size = 3) - (RelativePositioning::MAX_POSITION - size)..RelativePositioning::MAX_POSITION - end - - def run_at_start(size = 3) - (RelativePositioning::MIN_POSITION..).take(size) end end diff --git a/yarn.lock b/yarn.lock index 1e928bc3c57..e49e362649d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -848,10 +848,10 @@ resolved "https://registry.yarnpkg.com/@gitlab/svgs/-/svgs-1.158.0.tgz#300d416184a2b0e05f15a96547f726e1825b08a1" integrity sha512-5OJl+7TsXN9PJhY6/uwi+mTwmDZa9n/6119rf77orQ/joFYUypaYhBmy/1TcKVPsy5Zs6KCxE1kmGsfoXc1TYA== -"@gitlab/ui@20.1.1": - version "20.1.1" - resolved "https://registry.yarnpkg.com/@gitlab/ui/-/ui-20.1.1.tgz#990ce3a0883af5c62b0f56be1e0b244b918a9159" - integrity sha512-xtWdvzC33p8i76afHtnQKuUN7fGWV89uIKfIf9/WyygXZqUFKbSW076m/9iLRxHaCYNW7ucJe3fbEW+iAgWcuA== +"@gitlab/ui@20.3.1": + version "20.3.1" + resolved "https://registry.yarnpkg.com/@gitlab/ui/-/ui-20.3.1.tgz#4f29f9c16b34303074228081264415c3cd1e04de" + integrity sha512-CwxTKzvyVU4s25RCcfa4NBSxnRqQ/zHrYsAyBOJdK7uTDcuoPh6UqvXw4U0ghyIExRtTsF9GCWQJNYxcRT6p/g== dependencies: "@babel/standalone" "^7.0.0" "@gitlab/vue-toasted" "^1.3.0" |