diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-05-04 06:10:10 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-05-04 06:10:10 +0000 |
commit | 2fa68d3a97fd31bf469050e130f0fc95e8944316 (patch) | |
tree | 5c00585c55c44917765c152426cb58c803b4f57f | |
parent | 21be9646a94e2c145897e25d9c521523d55e1614 (diff) | |
download | gitlab-ce-2fa68d3a97fd31bf469050e130f0fc95e8944316.tar.gz |
Add latest changes from gitlab-org/gitlab@master
55 files changed, 3589 insertions, 48 deletions
diff --git a/app/assets/javascripts/alert_management/components/alert_management_list.vue b/app/assets/javascripts/alert_management/components/alert_management_list.vue index 5ea51bef496..1e0cbfbf125 100644 --- a/app/assets/javascripts/alert_management/components/alert_management_list.vue +++ b/app/assets/javascripts/alert_management/components/alert_management_list.vue @@ -1,5 +1,13 @@ <script> -import { GlEmptyState, GlDeprecatedButton, GlLoadingIcon, GlTable, GlAlert } from '@gitlab/ui'; +import { + GlEmptyState, + GlDeprecatedButton, + GlLoadingIcon, + GlTable, + GlAlert, + GlNewDropdown, + GlNewDropdownItem, +} from '@gitlab/ui'; import { s__ } from '~/locale'; import TimeAgo from '~/vue_shared/components/time_ago_tooltip.vue'; import getAlerts from '../graphql/queries/getAlerts.query.graphql'; @@ -42,6 +50,11 @@ export default { label: s__('AlertManagement|Status'), }, ], + statuses: { + triggered: s__('AlertManagement|Triggered'), + acknowledged: s__('AlertManagement|Acknowledged'), + resolved: s__('AlertManagement|Resolved'), + }, components: { GlEmptyState, GlLoadingIcon, @@ -49,6 +62,8 @@ export default { GlAlert, GlDeprecatedButton, TimeAgo, + GlNewDropdown, + GlNewDropdownItem, }, props: { projectPath: { @@ -140,6 +155,13 @@ export default { <template #cell(title)="{ item }"> <div class="gl-max-w-full text-truncate">{{ item.title }}</div> </template> + <template #cell(status)="{ item }"> + <gl-new-dropdown class="w-100" :text="item.status"> + <gl-new-dropdown-item v-for="(label, field) in $options.statuses" :key="field"> + {{ label }} + </gl-new-dropdown-item> + </gl-new-dropdown> + </template> <template #empty> {{ s__('AlertManagement|No alerts to display.') }} diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 3d53ad1a29f..95b4d5e5658 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -154,7 +154,10 @@ class Projects::IssuesController < Projects::ApplicationController end def related_branches - @related_branches = Issues::RelatedBranchesService.new(project, current_user).execute(issue) + @related_branches = Issues::RelatedBranchesService + .new(project, current_user) + .execute(issue) + .map { |branch| branch.merge(link: branch_link(branch)) } respond_to do |format| format.json do @@ -306,6 +309,10 @@ class Projects::IssuesController < Projects::ApplicationController private + def branch_link(branch) + project_compare_path(project, from: project.default_branch, to: branch[:name]) + end + def create_rate_limit key = :issues_create diff --git a/app/models/design_management.rb b/app/models/design_management.rb new file mode 100644 index 00000000000..81e170f7e59 --- /dev/null +++ b/app/models/design_management.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module DesignManagement + DESIGN_IMAGE_SIZES = %w(v432x230).freeze + + def self.designs_directory + 'designs' + end + + def self.table_name_prefix + 'design_management_' + end +end diff --git a/app/models/design_management/action.rb b/app/models/design_management/action.rb new file mode 100644 index 00000000000..8fe7d7c577c --- /dev/null +++ b/app/models/design_management/action.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module DesignManagement + class Action < ApplicationRecord + include WithUploads + + self.table_name = "#{DesignManagement.table_name_prefix}designs_versions" + + mount_uploader :image_v432x230, DesignManagement::DesignV432x230Uploader + + belongs_to :design, class_name: "DesignManagement::Design", inverse_of: :actions + belongs_to :version, class_name: "DesignManagement::Version", inverse_of: :actions + + enum event: { creation: 0, modification: 1, deletion: 2 } + + # we assume sequential ordering. + scope :ordered, -> { order(version_id: :asc) } + + # For each design, only select the most recent action + scope :most_recent, -> do + selection = Arel.sql("DISTINCT ON (#{table_name}.design_id) #{table_name}.*") + + order(arel_table[:design_id].asc, arel_table[:version_id].desc).select(selection) + end + + # Find all records created before or at the given version, or all if nil + scope :up_to_version, ->(version = nil) do + case version + when nil + all + when DesignManagement::Version + where(arel_table[:version_id].lteq(version.id)) + when ::Gitlab::Git::COMMIT_ID + versions = DesignManagement::Version.arel_table + subquery = versions.project(versions[:id]).where(versions[:sha].eq(version)) + where(arel_table[:version_id].lteq(subquery)) + else + raise ArgumentError, "Expected a DesignManagement::Version, got #{version}" + end + end + end +end diff --git a/app/models/design_management/design.rb b/app/models/design_management/design.rb new file mode 100644 index 00000000000..e9b69eab7a7 --- /dev/null +++ b/app/models/design_management/design.rb @@ -0,0 +1,266 @@ +# frozen_string_literal: true + +module DesignManagement + class Design < ApplicationRecord + include Importable + include Noteable + include Gitlab::FileTypeDetection + include Gitlab::Utils::StrongMemoize + include Referable + include Mentionable + include WhereComposite + + belongs_to :project, inverse_of: :designs + belongs_to :issue + + has_many :actions + has_many :versions, through: :actions, class_name: 'DesignManagement::Version', inverse_of: :designs + # This is a polymorphic association, so we can't count on FK's to delete the + # data + has_many :notes, as: :noteable, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent + has_many :user_mentions, class_name: 'DesignUserMention', dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent + + validates :project, :filename, presence: true + validates :issue, presence: true, unless: :importing? + validates :filename, uniqueness: { scope: :issue_id } + validate :validate_file_is_image + + alias_attribute :title, :filename + + # Pre-fetching scope to include the data necessary to construct a + # reference using `to_reference`. + scope :for_reference, -> { includes(issue: [{ project: [:route, :namespace] }]) } + + # A design can be uniquely identified by issue_id and filename + # Takes one or more sets of composite IDs of the form: + # `{issue_id: Integer, filename: String}`. + # + # @see WhereComposite::where_composite + # + # e.g: + # + # by_issue_id_and_filename(issue_id: 1, filename: 'homescreen.jpg') + # by_issue_id_and_filename([]) # returns ActiveRecord::NullRelation + # by_issue_id_and_filename([ + # { issue_id: 1, filename: 'homescreen.jpg' }, + # { issue_id: 2, filename: 'homescreen.jpg' }, + # { issue_id: 1, filename: 'menu.png' } + # ]) + # + scope :by_issue_id_and_filename, ->(composites) do + where_composite(%i[issue_id filename], composites) + end + + # Find designs visible at the given version + # + # @param version [nil, DesignManagement::Version]: + # the version at which the designs must be visible + # Passing `nil` is the same as passing the most current version + # + # Restricts to designs + # - created at least *before* the given version + # - not deleted as of the given version. + # + # As a query, we ascertain this by finding the last event prior to + # (or equal to) the cut-off, and seeing whether that version was a deletion. + scope :visible_at_version, -> (version) do + deletion = ::DesignManagement::Action.events[:deletion] + designs = arel_table + actions = ::DesignManagement::Action + .most_recent.up_to_version(version) + .arel.as('most_recent_actions') + + join = designs.join(actions) + .on(actions[:design_id].eq(designs[:id])) + + joins(join.join_sources).where(actions[:event].not_eq(deletion)).order(:id) + end + + scope :with_filename, -> (filenames) { where(filename: filenames) } + scope :on_issue, ->(issue) { where(issue_id: issue) } + + # Scope called by our REST API to avoid N+1 problems + scope :with_api_entity_associations, -> { preload(:issue) } + + # A design is current if the most recent event is not a deletion + scope :current, -> { visible_at_version(nil) } + + def status + if new_design? + :new + elsif deleted? + :deleted + else + :current + end + end + + def deleted? + most_recent_action&.deletion? + end + + # A design is visible_in? a version if: + # * it was created before that version + # * the most recent action before the version was not a deletion + def visible_in?(version) + map = strong_memoize(:visible_in) do + Hash.new do |h, k| + h[k] = self.class.visible_at_version(k).where(id: id).exists? + end + end + + map[version] + end + + def most_recent_action + strong_memoize(:most_recent_action) { actions.ordered.last } + end + + # A reference for a design is the issue reference, indexed by the filename + # with an optional infix when full. + # + # e.g. + # #123[homescreen.png] + # other-project#72[sidebar.jpg] + # #38/designs[transition.gif] + # #12["filename with [] in it.jpg"] + def to_reference(from = nil, full: false) + infix = full ? '/designs' : '' + totally_simple = %r{ \A #{self.class.simple_file_name} \z }x + safe_name = if totally_simple.match?(filename) + filename + elsif filename =~ /[<>]/ + %Q{base64:#{Base64.strict_encode64(filename)}} + else + escaped = filename.gsub(%r{[\\"]}) { |x| "\\#{x}" } + %Q{"#{escaped}"} + end + + "#{issue.to_reference(from, full: full)}#{infix}[#{safe_name}]" + end + + def self.reference_pattern + @reference_pattern ||= begin + # Filenames can be escaped with double quotes to name filenames + # that include square brackets, or other special characters + %r{ + #{Issue.reference_pattern} + (\/designs)? + \[ + (?<design> #{simple_file_name} | #{quoted_file_name} | #{base_64_encoded_name}) + \] + }x + end + end + + def self.simple_file_name + %r{ + (?<simple_file_name> + ( \w | [_:,'-] | \. | \s )+ + \. + \w+ + ) + }x + end + + def self.base_64_encoded_name + %r{ + base64: + (?<base_64_encoded_name> + [A-Za-z0-9+\n]+ + =? + ) + }x + end + + def self.quoted_file_name + %r{ + " + (?<escaped_filename> + (\\ \\ | \\ " | [^"\\])+ + ) + " + }x + end + + def self.link_reference_pattern + @link_reference_pattern ||= begin + exts = SAFE_IMAGE_EXT + DANGEROUS_IMAGE_EXT + path_segment = %r{issues/#{Gitlab::Regex.issue}/designs} + filename_pattern = %r{(?<simple_file_name>[a-z0-9_=-]+\.(#{exts.join('|')}))}i + + super(path_segment, filename_pattern) + end + end + + def to_ability_name + 'design' + end + + def description + '' + end + + def new_design? + strong_memoize(:new_design) { actions.none? } + end + + def full_path + @full_path ||= File.join(DesignManagement.designs_directory, "issue-#{issue.iid}", filename) + end + + def diff_refs + strong_memoize(:diff_refs) { head_version&.diff_refs } + end + + def clear_version_cache + [versions, actions].each(&:reset) + %i[new_design diff_refs head_sha visible_in most_recent_action].each do |key| + clear_memoization(key) + end + end + + def repository + project.design_repository + end + + def user_notes_count + user_notes_count_service.count + end + + def after_note_changed(note) + user_notes_count_service.delete_cache unless note.system? + end + alias_method :after_note_created, :after_note_changed + alias_method :after_note_destroyed, :after_note_changed + + private + + def head_version + strong_memoize(:head_sha) { versions.ordered.first } + end + + def allow_dangerous_images? + Feature.enabled?(:design_management_allow_dangerous_images, project) + end + + def valid_file_extensions + allow_dangerous_images? ? (SAFE_IMAGE_EXT + DANGEROUS_IMAGE_EXT) : SAFE_IMAGE_EXT + end + + def validate_file_is_image + unless image? || (dangerous_image? && allow_dangerous_images?) + message = _('does not have a supported extension. Only %{extension_list} are supported') % { + extension_list: valid_file_extensions.to_sentence + } + errors.add(:filename, message) + end + end + + def user_notes_count_service + strong_memoize(:user_notes_count_service) do + ::DesignManagement::DesignUserNotesCountService.new(self) # rubocop: disable CodeReuse/ServiceClass + end + end + end +end diff --git a/app/models/design_management/design_action.rb b/app/models/design_management/design_action.rb new file mode 100644 index 00000000000..22baa916296 --- /dev/null +++ b/app/models/design_management/design_action.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +module DesignManagement + # Parameter object which is a tuple of the database record and the + # last gitaly call made to change it. This serves to perform the + # logical mapping from git action to database representation. + class DesignAction + include ActiveModel::Validations + + EVENT_FOR_GITALY_ACTION = { + create: DesignManagement::Action.events[:creation], + update: DesignManagement::Action.events[:modification], + delete: DesignManagement::Action.events[:deletion] + }.freeze + + attr_reader :design, :action, :content + + delegate :issue_id, to: :design + + validates :design, presence: true + validates :action, presence: true, inclusion: { in: EVENT_FOR_GITALY_ACTION.keys } + validates :content, + absence: { if: :forbids_content?, + message: 'this action forbids content' }, + presence: { if: :needs_content?, + message: 'this action needs content' } + + # Parameters: + # - design [DesignManagement::Design]: the design that was changed + # - action [Symbol]: the action that gitaly performed + def initialize(design, action, content = nil) + @design, @action, @content = design, action, content + validate! + end + + def row_attrs(version) + { design_id: design.id, version_id: version.id, event: event } + end + + def gitaly_action + { action: action, file_path: design.full_path, content: content }.compact + end + + # This action has been performed - do any post-creation actions + # such as clearing method caches. + def performed + design.clear_version_cache + end + + private + + def needs_content? + action != :delete + end + + def forbids_content? + action == :delete + end + + def event + EVENT_FOR_GITALY_ACTION[action] + end + end +end diff --git a/app/models/design_management/design_at_version.rb b/app/models/design_management/design_at_version.rb new file mode 100644 index 00000000000..b4cafb93c2c --- /dev/null +++ b/app/models/design_management/design_at_version.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +# Tuple of design and version +# * has a composite ID, with lazy_find +module DesignManagement + class DesignAtVersion + include ActiveModel::Validations + include GlobalID::Identification + include Gitlab::Utils::StrongMemoize + + attr_reader :version + attr_reader :design + + validates :version, presence: true + validates :design, presence: true + + validate :design_and_version_belong_to_the_same_issue + validate :design_and_version_have_issue_id + + def initialize(design: nil, version: nil) + @design, @version = design, version + end + + def self.instantiate(attrs) + new(attrs).tap { |obj| obj.validate! } + end + + # The ID, needed by GraphQL types and as part of the Lazy-fetch + # protocol, includes information about both the design and the version. + # + # The particular format is not interesting, and should be treated as opaque + # by all callers. + def id + "#{design.id}.#{version.id}" + end + + def ==(other) + return false unless other && self.class == other.class + + other.id == id + end + + alias_method :eql?, :== + + def self.lazy_find(id) + BatchLoader.for(id).batch do |ids, callback| + find(ids).each do |record| + callback.call(record.id, record) + end + end + end + + def self.find(ids) + pairs = ids.map { |id| id.split('.').map(&:to_i) } + + design_ids = pairs.map(&:first).uniq + version_ids = pairs.map(&:second).uniq + + designs = ::DesignManagement::Design + .where(id: design_ids) + .index_by(&:id) + + versions = ::DesignManagement::Version + .where(id: version_ids) + .index_by(&:id) + + pairs.map do |(design_id, version_id)| + design = designs[design_id] + version = versions[version_id] + + obj = new(design: design, version: version) + + obj if obj.valid? + end.compact + end + + def status + if not_created_yet? + :not_created_yet + elsif deleted? + :deleted + else + :current + end + end + + def deleted? + action&.deletion? + end + + def not_created_yet? + action.nil? + end + + private + + def action + strong_memoize(:most_recent_action) do + ::DesignManagement::Action + .most_recent.up_to_version(version) + .find_by(design: design) + end + end + + def design_and_version_belong_to_the_same_issue + id_a, id_b = [design, version].map { |obj| obj&.issue_id } + + return if id_a == id_b + + errors.add(:issue, 'must be the same on design and version') + end + + def design_and_version_have_issue_id + return if [design, version].all? { |obj| obj.try(:issue_id).present? } + + errors.add(:issue, 'must be present on both design and version') + end + end +end diff --git a/app/models/design_management/design_collection.rb b/app/models/design_management/design_collection.rb new file mode 100644 index 00000000000..18d1541e9c7 --- /dev/null +++ b/app/models/design_management/design_collection.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module DesignManagement + class DesignCollection + attr_reader :issue + + delegate :designs, :project, to: :issue + + def initialize(issue) + @issue = issue + end + + def find_or_create_design!(filename:) + designs.find { |design| design.filename == filename } || + designs.safe_find_or_create_by!(project: project, filename: filename) + end + + def versions + @versions ||= DesignManagement::Version.for_designs(designs) + end + + def repository + project.design_repository + end + + def designs_by_filename(filenames) + designs.current.where(filename: filenames) + end + end +end diff --git a/app/models/design_management/repository.rb b/app/models/design_management/repository.rb new file mode 100644 index 00000000000..985d6317d5d --- /dev/null +++ b/app/models/design_management/repository.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module DesignManagement + class Repository < ::Repository + extend ::Gitlab::Utils::Override + + # We define static git attributes for the design repository as this + # repository is entirely GitLab-managed rather than user-facing. + # + # Enable all uploaded files to be stored in LFS. + MANAGED_GIT_ATTRIBUTES = <<~GA.freeze + /#{DesignManagement.designs_directory}/* filter=lfs diff=lfs merge=lfs -text + GA + + def initialize(project) + full_path = project.full_path + Gitlab::GlRepository::DESIGN.path_suffix + disk_path = project.disk_path + Gitlab::GlRepository::DESIGN.path_suffix + + super(full_path, project, shard: project.repository_storage, disk_path: disk_path, repo_type: Gitlab::GlRepository::DESIGN) + end + + # Override of a method called on Repository instances but sent via + # method_missing to Gitlab::Git::Repository where it is defined + def info_attributes + @info_attributes ||= Gitlab::Git::AttributesParser.new(MANAGED_GIT_ATTRIBUTES) + end + + # Override of a method called on Repository instances but sent via + # method_missing to Gitlab::Git::Repository where it is defined + def attributes(path) + info_attributes.attributes(path) + end + + # Override of a method called on Repository instances but sent via + # method_missing to Gitlab::Git::Repository where it is defined + def gitattribute(path, name) + attributes(path)[name] + end + + # Override of a method called on Repository instances but sent via + # method_missing to Gitlab::Git::Repository where it is defined + def attributes_at(_ref = nil) + info_attributes + end + + override :copy_gitattributes + def copy_gitattributes(_ref = nil) + true + end + end +end diff --git a/app/models/design_management/version.rb b/app/models/design_management/version.rb new file mode 100644 index 00000000000..6be98fe3d44 --- /dev/null +++ b/app/models/design_management/version.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +module DesignManagement + class Version < ApplicationRecord + include Importable + include ShaAttribute + include AfterCommitQueue + include Gitlab::Utils::StrongMemoize + extend Gitlab::ExclusiveLeaseHelpers + + NotSameIssue = Class.new(StandardError) + + class CouldNotCreateVersion < StandardError + attr_reader :sha, :issue_id, :actions + + def initialize(sha, issue_id, actions) + @sha, @issue_id, @actions = sha, issue_id, actions + end + + def message + "could not create version from commit: #{sha}" + end + + def sentry_extra_data + { + sha: sha, + issue_id: issue_id, + design_ids: actions.map { |a| a.design.id } + } + end + end + + belongs_to :issue + belongs_to :author, class_name: 'User' + has_many :actions + has_many :designs, + through: :actions, + class_name: "DesignManagement::Design", + source: :design, + inverse_of: :versions + + validates :designs, presence: true, unless: :importing? + validates :sha, presence: true + validates :sha, uniqueness: { case_sensitive: false, scope: :issue_id } + validates :author, presence: true + # We are not validating the issue object as it incurs an extra query to fetch + # the record from the DB. Instead, we rely on the foreign key constraint to + # ensure referential integrity. + validates :issue_id, presence: true, unless: :importing? + + sha_attribute :sha + + delegate :project, to: :issue + + scope :for_designs, -> (designs) do + where(id: ::DesignManagement::Action.where(design_id: designs).select(:version_id)).distinct + end + scope :earlier_or_equal_to, -> (version) { where("(#{table_name}.id) <= ?", version) } # rubocop:disable GitlabSecurity/SqlInjection + scope :ordered, -> { order(id: :desc) } + scope :for_issue, -> (issue) { where(issue: issue) } + scope :by_sha, -> (sha) { where(sha: sha) } + + # This is the one true way to create a Version. + # + # This method means you can avoid the paradox of versions being invalid without + # designs, and not being able to add designs without a saved version. Also this + # method inserts designs in bulk, rather than one by one. + # + # Before calling this method, callers must guard against concurrent + # modification by obtaining the lock on the design repository. See: + # `DesignManagement::Version.with_lock`. + # + # Parameters: + # - design_actions [DesignManagement::DesignAction]: + # the actions that have been performed in the repository. + # - sha [String]: + # the SHA of the commit that performed them + # - author [User]: + # the user who performed the commit + # returns [DesignManagement::Version] + def self.create_for_designs(design_actions, sha, author) + issue_id, not_uniq = design_actions.map(&:issue_id).compact.uniq + raise NotSameIssue, 'All designs must belong to the same issue!' if not_uniq + + transaction do + version = new(sha: sha, issue_id: issue_id, author: author) + version.save(validate: false) # We need it to have an ID. Validate later when designs are present + + rows = design_actions.map { |action| action.row_attrs(version) } + + Gitlab::Database.bulk_insert(::DesignManagement::Action.table_name, rows) + version.designs.reset + version.validate! + design_actions.each(&:performed) + + version + end + rescue + raise CouldNotCreateVersion.new(sha, issue_id, design_actions) + end + + CREATION_TTL = 5.seconds + RETRY_DELAY = ->(num) { 0.2.seconds * num**2 } + + def self.with_lock(project_id, repository, &block) + key = "with_lock:#{name}:{#{project_id}}" + + in_lock(key, ttl: CREATION_TTL, retries: 5, sleep_sec: RETRY_DELAY) do |_retried| + repository.create_if_not_exists + yield + end + end + + def designs_by_event + actions + .includes(:design) + .group_by(&:event) + .transform_values { |group| group.map(&:design) } + end + + def author + super || (commit_author if persisted?) + end + + def diff_refs + strong_memoize(:diff_refs) { commit&.diff_refs } + end + + def reset + %i[diff_refs commit].each { |k| clear_memoization(k) } + super + end + + private + + def commit_author + commit&.author + end + + def commit + strong_memoize(:commit) { issue.project.design_repository.commit(sha) } + end + end +end diff --git a/app/models/design_user_mention.rb b/app/models/design_user_mention.rb new file mode 100644 index 00000000000..baf4db29a0f --- /dev/null +++ b/app/models/design_user_mention.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +class DesignUserMention < UserMention + belongs_to :design, class_name: 'DesignManagement::Design' + belongs_to :note +end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index e3df61dadae..ff39dbb59f3 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -9,7 +9,7 @@ class DiffNote < Note include Gitlab::Utils::StrongMemoize def self.noteable_types - %w(MergeRequest Commit) + %w(MergeRequest Commit DesignManagement::Design) end validates :original_position, presence: true @@ -60,6 +60,8 @@ class DiffNote < Note # Returns the diff file from `position` def latest_diff_file strong_memoize(:latest_diff_file) do + next if for_design? + position.diff_file(repository) end end @@ -67,6 +69,8 @@ class DiffNote < Note # Returns the diff file from `original_position` def diff_file strong_memoize(:diff_file) do + next if for_design? + enqueue_diff_file_creation_job if should_create_diff_file? fetch_diff_file @@ -145,7 +149,7 @@ class DiffNote < Note end def supported? - for_commit? || self.noteable.has_complete_diff_refs? + for_commit? || for_design? || self.noteable.has_complete_diff_refs? end def set_line_code @@ -184,5 +188,3 @@ class DiffNote < Note noteable.respond_to?(:repository) ? noteable.repository : project.repository end end - -DiffNote.prepend_if_ee('::EE::DiffNote') diff --git a/app/models/issue.rb b/app/models/issue.rb index 7e303bc257a..82643d8f5d6 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -49,6 +49,12 @@ class Issue < ApplicationRecord has_many :zoom_meetings has_many :user_mentions, class_name: "IssueUserMention", dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_many :sent_notifications, as: :noteable + has_many :designs, class_name: 'DesignManagement::Design', inverse_of: :issue + has_many :design_versions, class_name: 'DesignManagement::Version', inverse_of: :issue do + def most_recent + ordered.first + end + end has_one :sentry_issue has_one :alert_management_alert, class_name: 'AlertManagement::Alert' @@ -334,6 +340,10 @@ class Issue < ApplicationRecord previous_changes['updated_at']&.first || updated_at end + def design_collection + @design_collection ||= ::DesignManagement::DesignCollection.new(self) + end + private def ensure_metrics diff --git a/app/models/note.rb b/app/models/note.rb index a2a711c987f..9d0cd30f5dc 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -279,6 +279,10 @@ class Note < ApplicationRecord !for_personal_snippet? end + def for_design? + noteable_type == DesignManagement::Design.name + end + def for_issuable? for_issue? || for_merge_request? end diff --git a/app/models/project.rb b/app/models/project.rb index 502d3391d63..bd1785bc620 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -215,6 +215,7 @@ class Project < ApplicationRecord has_many :protected_branches has_many :protected_tags has_many :repository_languages, -> { order "share DESC" } + has_many :designs, inverse_of: :project, class_name: 'DesignManagement::Design' has_many :project_authorizations has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User' @@ -791,6 +792,11 @@ class Project < ApplicationRecord Feature.enabled?(:jira_issue_import, self, default_enabled: true) end + # LFS and hashed repository storage are required for using Design Management. + def design_management_enabled? + lfs_enabled? && hashed_storage?(:repository) + end + def team @team ||= ProjectTeam.new(self) end @@ -799,6 +805,12 @@ class Project < ApplicationRecord @repository ||= Repository.new(full_path, self, shard: repository_storage, disk_path: disk_path) end + def design_repository + strong_memoize(:design_repository) do + DesignManagement::Repository.new(self) + end + end + def cleanup @repository = nil end diff --git a/app/services/design_management/design_user_notes_count_service.rb b/app/services/design_management/design_user_notes_count_service.rb new file mode 100644 index 00000000000..e49914ea6d3 --- /dev/null +++ b/app/services/design_management/design_user_notes_count_service.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module DesignManagement + # Service class for counting and caching the number of unresolved + # notes of a Design + class DesignUserNotesCountService < ::BaseCountService + # The version of the cache format. This should be bumped whenever the + # underlying logic changes. This removes the need for explicitly flushing + # all caches. + VERSION = 1 + + def initialize(design) + @design = design + end + + def relation_for_count + design.notes.user + end + + def raw? + # Since we're storing simple integers we don't need all of the + # additional Marshal data Rails includes by default. + true + end + + def cache_key + ['designs', 'notes_count', VERSION, design.id] + end + + private + + attr_reader :design + end +end diff --git a/app/services/issues/related_branches_service.rb b/app/services/issues/related_branches_service.rb index 76af482b7ac..46076218857 100644 --- a/app/services/issues/related_branches_service.rb +++ b/app/services/issues/related_branches_service.rb @@ -5,11 +5,29 @@ module Issues class RelatedBranchesService < Issues::BaseService def execute(issue) - branches_with_iid_of(issue) - branches_with_merge_request_for(issue) + branch_names = branches_with_iid_of(issue) - branches_with_merge_request_for(issue) + branch_names.map { |branch_name| branch_data(branch_name) } end private + def branch_data(branch_name) + { + name: branch_name, + pipeline_status: pipeline_status(branch_name) + } + end + + def pipeline_status(branch_name) + branch = project.repository.find_branch(branch_name) + target = branch&.dereferenced_target + + return unless target + + pipeline = project.pipeline_for(branch_name, target.sha) + pipeline.detailed_status(current_user) if can?(current_user, :read_pipeline, pipeline) + end + def branches_with_merge_request_for(issue) Issues::ReferencedMergeRequestsService .new(project, current_user) diff --git a/app/uploaders/design_management/design_v432x230_uploader.rb b/app/uploaders/design_management/design_v432x230_uploader.rb new file mode 100644 index 00000000000..ba48f381bbd --- /dev/null +++ b/app/uploaders/design_management/design_v432x230_uploader.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module DesignManagement + # This Uploader is used to generate and serve the smaller versions of + # the design files. + # + # The original (full-sized) design files are stored in Git LFS, and so + # have a different uploader, `LfsObjectUploader`. + class DesignV432x230Uploader < GitlabUploader + include CarrierWave::MiniMagick + include RecordsUploads::Concern + include ObjectStorage::Concern + prepend ObjectStorage::Extension::RecordsUploads + + # We choose not to resize `image/ico` as we assume there will be no + # benefit in generating an 432x230 sized icon. + # + # We currently cannot resize `image/tiff`. + # See https://gitlab.com/gitlab-org/gitlab/issues/207740 + # + # We currently choose not to resize `image/svg+xml` for security reasons. + # See https://gitlab.com/gitlab-org/gitlab/issues/207740#note_302766171 + MIME_TYPE_WHITELIST = %w(image/png image/jpeg image/bmp image/gif).freeze + + process resize_to_fit: [432, 230] + + # Allow CarrierWave to reject files without correct mimetypes. + def content_type_whitelist + MIME_TYPE_WHITELIST + end + + # Override `GitlabUploader` and always return false, otherwise local + # `LfsObject` files would be deleted. + # https://github.com/carrierwaveuploader/carrierwave/blob/f84672a/lib/carrierwave/uploader/cache.rb#L131-L135 + def move_to_cache + false + end + + private + + def dynamic_segment + File.join(model.class.underscore, mounted_as.to_s, model.id.to_s) + end + end +end diff --git a/app/views/projects/graphs/charts.html.haml b/app/views/projects/graphs/charts.html.haml index 19fe7ba4360..f820d3f43cb 100644 --- a/app/views/projects/graphs/charts.html.haml +++ b/app/views/projects/graphs/charts.html.haml @@ -1,5 +1,9 @@ - page_title _("Repository Analytics") +.mb-3 + %h3 + = _("Repository Analytics") + .repo-charts %h4.sub-header = _("Programming languages used in this repository") diff --git a/app/views/projects/issues/_related_branches.html.haml b/app/views/projects/issues/_related_branches.html.haml index 69b030ed76a..0604e89be6e 100644 --- a/app/views/projects/issues/_related_branches.html.haml +++ b/app/views/projects/issues/_related_branches.html.haml @@ -4,11 +4,9 @@ %ul.unstyled-list.related-merge-requests - @related_branches.each do |branch| %li - - target = @project.repository.find_branch(branch).dereferenced_target - - pipeline = @project.pipeline_for(branch, target.sha) if target - - if can?(current_user, :read_pipeline, pipeline) + - if branch[:pipeline_status].present? %span.related-branch-ci-status - = render 'ci/status/icon', status: pipeline.detailed_status(current_user) + = render 'ci/status/icon', status: branch[:pipeline_status] %span.related-branch-info %strong - = link_to branch, project_compare_path(@project, from: @project.default_branch, to: branch), class: "ref-name" + = link_to branch[:name], branch[:link], class: "ref-name" diff --git a/changelogs/unreleased/mw-ca-add-title.yml b/changelogs/unreleased/mw-ca-add-title.yml new file mode 100644 index 00000000000..5282b17bc04 --- /dev/null +++ b/changelogs/unreleased/mw-ca-add-title.yml @@ -0,0 +1,5 @@ +--- +title: 'Contribution Analytics: Add title to page' +merge_request: 30842 +author: +type: added diff --git a/changelogs/unreleased/mw-ra-add-title.yml b/changelogs/unreleased/mw-ra-add-title.yml new file mode 100644 index 00000000000..129ef57a6b1 --- /dev/null +++ b/changelogs/unreleased/mw-ra-add-title.yml @@ -0,0 +1,5 @@ +--- +title: 'Repository Analytics: Add title to page' +merge_request: 30855 +author: +type: added diff --git a/lib/gitlab/git_access_design.rb b/lib/gitlab/git_access_design.rb new file mode 100644 index 00000000000..36604bd0b3b --- /dev/null +++ b/lib/gitlab/git_access_design.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Gitlab + class GitAccessDesign < GitAccess + def check(_cmd, _changes) + check_protocol! + check_can_create_design! + + success_result + end + + private + + def check_protocol! + if protocol != 'web' + raise ::Gitlab::GitAccess::ForbiddenError, "Designs are only accessible using the web interface" + end + end + + def check_can_create_design! + unless user&.can?(:create_design, project) + raise ::Gitlab::GitAccess::ForbiddenError, "You are not allowed to manage designs of this project" + end + end + end +end + +Gitlab::GitAccessDesign.prepend_if_ee('EE::Gitlab::GitAccessDesign') diff --git a/lib/gitlab/gl_repository.rb b/lib/gitlab/gl_repository.rb index 26440e6f82d..8abefad1ef3 100644 --- a/lib/gitlab/gl_repository.rb +++ b/lib/gitlab/gl_repository.rb @@ -23,11 +23,18 @@ module Gitlab project_resolver: -> (snippet) { snippet&.project }, guest_read_ability: :read_snippet ).freeze + DESIGN = ::Gitlab::GlRepository::RepoType.new( + name: :design, + access_checker_class: ::Gitlab::GitAccessDesign, + repository_resolver: -> (project) { ::DesignManagement::Repository.new(project) }, + suffix: :design + ).freeze TYPES = { PROJECT.name.to_s => PROJECT, WIKI.name.to_s => WIKI, - SNIPPET.name.to_s => SNIPPET + SNIPPET.name.to_s => SNIPPET, + DESIGN.name.to_s => DESIGN }.freeze def self.types @@ -58,5 +65,3 @@ module Gitlab private_class_method :instance end end - -Gitlab::GlRepository.prepend_if_ee('::EE::Gitlab::GlRepository') diff --git a/lib/gitlab/gl_repository/repo_type.rb b/lib/gitlab/gl_repository/repo_type.rb index 052ce578881..64c329b15ae 100644 --- a/lib/gitlab/gl_repository/repo_type.rb +++ b/lib/gitlab/gl_repository/repo_type.rb @@ -57,6 +57,10 @@ module Gitlab self == SNIPPET end + def design? + self == DESIGN + end + def path_suffix suffix ? ".#{suffix}" : '' end @@ -87,5 +91,3 @@ module Gitlab end end end - -Gitlab::GlRepository::RepoType.prepend_if_ee('EE::Gitlab::GlRepository::RepoType') diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 262a7e227ab..900adb03a03 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1695,6 +1695,9 @@ msgstr[1] "" msgid "Alert Details" msgstr "" +msgid "AlertManagement|Acknowledged" +msgstr "" + msgid "AlertManagement|Alert" msgstr "" @@ -1731,6 +1734,9 @@ msgstr "" msgid "AlertManagement|Overview" msgstr "" +msgid "AlertManagement|Resolved" +msgstr "" + msgid "AlertManagement|Severity" msgstr "" @@ -1749,6 +1755,9 @@ msgstr "" msgid "AlertManagement|There was an error displaying the alerts. Confirm your endpoint's configuration details to ensure alerts appear." msgstr "" +msgid "AlertManagement|Triggered" +msgstr "" + msgid "AlertService|%{linkStart}Learn more%{linkEnd} about configuring this endpoint to receive alerts." msgstr "" diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 35b43afbcf3..a2df023fe4c 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -243,6 +243,91 @@ describe Projects::IssuesController do end end + describe '#related_branches' do + subject { get :related_branches, params: params, format: :json } + + before do + sign_in(user) + project.add_developer(developer) + end + + let(:developer) { user } + let(:params) do + { + namespace_id: project.namespace, + project_id: project, + id: issue.iid + } + end + + context 'the current user cannot download code' do + it 'prevents access' do + allow(controller).to receive(:can?).with(any_args).and_return(true) + allow(controller).to receive(:can?).with(user, :download_code, project).and_return(false) + + subject + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'there are no related branches' do + it 'assigns empty arrays', :aggregate_failures do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(assigns(:related_branches)).to be_empty + expect(response).to render_template('projects/issues/_related_branches') + expect(json_response).to eq('html' => '') + end + end + + context 'there are related branches' do + let(:missing_branch) { "#{issue.to_branch_name}-missing" } + let(:unreadable_branch) { "#{issue.to_branch_name}-unreadable" } + let(:pipeline) { build(:ci_pipeline, :success, project: project) } + let(:master_branch) { 'master' } + + let(:related_branches) do + [ + branch_info(issue.to_branch_name, pipeline.detailed_status(user)), + branch_info(missing_branch, nil), + branch_info(unreadable_branch, nil) + ] + end + + def branch_info(name, status) + { + name: name, + link: controller.project_compare_path(project, from: master_branch, to: name), + pipeline_status: status + } + end + + before do + allow(controller).to receive(:find_routable!) + .with(Project, project.full_path, any_args).and_return(project) + allow(project).to receive(:default_branch).and_return(master_branch) + allow_next_instance_of(Issues::RelatedBranchesService) do |service| + allow(service).to receive(:execute).and_return(related_branches) + end + end + + it 'finds and assigns the appropriate branch information', :aggregate_failures do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(assigns(:related_branches)).to contain_exactly( + branch_info(issue.to_branch_name, an_instance_of(Gitlab::Ci::Status::Success)), + branch_info(missing_branch, be_nil), + branch_info(unreadable_branch, be_nil) + ) + expect(response).to render_template('projects/issues/_related_branches') + expect(json_response).to match('html' => String) + end + end + end + # This spec runs as a request-style spec in order to invoke the # Rails router. A controller-style spec matches the wrong route, and # session['user_return_to'] becomes incorrect. diff --git a/spec/factories/design_management/actions.rb b/spec/factories/design_management/actions.rb new file mode 100644 index 00000000000..e2561f98f52 --- /dev/null +++ b/spec/factories/design_management/actions.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :design_action, class: 'DesignManagement::Action' do + design + association :version, factory: :design_version + event { :creation } + + trait :with_image_v432x230 do + image_v432x230 { fixture_file_upload('spec/fixtures/dk.png') } + end + end +end diff --git a/spec/factories/design_management/design_at_version.rb b/spec/factories/design_management/design_at_version.rb new file mode 100644 index 00000000000..b73df71595c --- /dev/null +++ b/spec/factories/design_management/design_at_version.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :design_at_version, class: 'DesignManagement::DesignAtVersion' do + skip_create # This is not an Active::Record model. + + design { nil } + + version { nil } + + transient do + issue { design&.issue || version&.issue || create(:issue) } + end + + initialize_with do + attrs = attributes.dup + attrs[:design] ||= create(:design, issue: issue) + attrs[:version] ||= create(:design_version, issue: issue) + + new(attrs) + end + end +end diff --git a/spec/factories/design_management/designs.rb b/spec/factories/design_management/designs.rb new file mode 100644 index 00000000000..59d4cc56f95 --- /dev/null +++ b/spec/factories/design_management/designs.rb @@ -0,0 +1,128 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :design, class: 'DesignManagement::Design' do + issue { create(:issue) } + project { issue&.project || create(:project) } + sequence(:filename) { |n| "homescreen-#{n}.jpg" } + + transient do + author { issue.author } + end + + trait :importing do + issue { nil } + + importing { true } + imported { false } + end + + trait :imported do + importing { false } + imported { true } + end + + create_versions = ->(design, evaluator, commit_version) do + unless evaluator.versions_count.zero? + project = design.project + issue = design.issue + repository = project.design_repository + repository.create_if_not_exists + dv_table_name = DesignManagement::Action.table_name + updates = [0, evaluator.versions_count - (evaluator.deleted ? 2 : 1)].max + + run_action = ->(action) do + sha = commit_version[action] + version = DesignManagement::Version.new(sha: sha, issue: issue, author: evaluator.author) + version.save(validate: false) # We need it to have an ID, validate later + Gitlab::Database.bulk_insert(dv_table_name, [action.row_attrs(version)]) + end + + # always a creation + run_action[DesignManagement::DesignAction.new(design, :create, evaluator.file)] + + # 0 or more updates + updates.times do + run_action[DesignManagement::DesignAction.new(design, :update, evaluator.file)] + end + + # and maybe a deletion + run_action[DesignManagement::DesignAction.new(design, :delete)] if evaluator.deleted + end + + design.clear_version_cache + end + + # Use this trait to build designs that are backed by Git LFS, committed + # to the repository, and with an LfsObject correctly created for it. + trait :with_lfs_file do + with_file + + transient do + raw_file { fixture_file_upload('spec/fixtures/dk.png', 'image/png') } + lfs_pointer { Gitlab::Git::LfsPointerFile.new(SecureRandom.random_bytes) } + file { lfs_pointer.pointer } + end + + after :create do |design, evaluator| + lfs_object = create(:lfs_object, file: evaluator.raw_file, oid: evaluator.lfs_pointer.sha256, size: evaluator.lfs_pointer.size) + create(:lfs_objects_project, project: design.project, lfs_object: lfs_object, repository_type: :design) + end + end + + # Use this trait if you want versions in a particular history, but don't + # want to pay for gitlay calls. + trait :with_versions do + transient do + deleted { false } + versions_count { 1 } + sequence(:file) { |n| "some-file-content-#{n}" } + end + + after :create do |design, evaluator| + counter = (1..).lazy + + # Just produce a SHA by hashing the action and a monotonic counter + commit_version = ->(action) do + Digest::SHA1.hexdigest("#{action.gitaly_action}.#{counter.next}") + end + + create_versions[design, evaluator, commit_version] + end + end + + # Use this trait to build designs that have commits in the repository + # and files that can be retrieved. + trait :with_file do + transient do + deleted { false } + versions_count { 1 } + file { File.join(Rails.root, 'spec/fixtures/dk.png') } + end + + after :create do |design, evaluator| + project = design.project + repository = project.design_repository + + commit_version = ->(action) do + repository.multi_action( + evaluator.author, + branch_name: 'master', + message: "#{action.action} for #{design.filename}", + actions: [action.gitaly_action] + ) + end + + create_versions[design, evaluator, commit_version] + end + end + + trait :with_smaller_image_versions do + with_lfs_file + + after :create do |design| + design.versions.each { |v| DesignManagement::GenerateImageVersionsService.new(v).execute } + end + end + end +end diff --git a/spec/factories/design_management/versions.rb b/spec/factories/design_management/versions.rb new file mode 100644 index 00000000000..878665e02e5 --- /dev/null +++ b/spec/factories/design_management/versions.rb @@ -0,0 +1,142 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :design_version, class: 'DesignManagement::Version' do + sequence(:sha) { |n| Digest::SHA1.hexdigest("commit-like-#{n}") } + issue { designs.first&.issue || create(:issue) } + author { issue&.author || create(:user) } + + transient do + designs_count { 1 } + created_designs { [] } + modified_designs { [] } + deleted_designs { [] } + end + + # Warning: this will intentionally result in an invalid version! + trait :empty do + designs_count { 0 } + end + + trait :importing do + issue { nil } + + designs_count { 0 } + importing { true } + imported { false } + end + + trait :imported do + importing { false } + imported { true } + end + + after(:build) do |version, evaluator| + # By default all designs are created_designs, so just add them. + specific_designs = [].concat( + evaluator.created_designs, + evaluator.modified_designs, + evaluator.deleted_designs + ) + version.designs += specific_designs + + unless evaluator.designs_count.zero? || version.designs.present? + version.designs << create(:design, issue: version.issue) + end + end + + after :create do |version, evaluator| + # FactoryBot does not like methods, so we use lambdas instead + events = DesignManagement::Action.events + + version.actions + .where(design_id: evaluator.modified_designs.map(&:id)) + .update_all(event: events[:modification]) + + version.actions + .where(design_id: evaluator.deleted_designs.map(&:id)) + .update_all(event: events[:deletion]) + + version.designs.reload + # Ensure version.issue == design.issue for all version.designs + version.designs.update_all(issue_id: version.issue_id) + + needed = evaluator.designs_count + have = version.designs.size + + create_list(:design, [0, needed - have].max, issue: version.issue).each do |d| + version.designs << d + end + + version.actions.reset + end + + # Use this trait to build versions with designs that are backed by Git LFS, committed + # to the repository, and with an LfsObject correctly created for it. + trait :with_lfs_file do + committed + + transient do + raw_file { fixture_file_upload('spec/fixtures/dk.png', 'image/png') } + lfs_pointer { Gitlab::Git::LfsPointerFile.new(SecureRandom.random_bytes) } + file { lfs_pointer.pointer } + end + + after :create do |version, evaluator| + lfs_object = create(:lfs_object, file: evaluator.raw_file, oid: evaluator.lfs_pointer.sha256, size: evaluator.lfs_pointer.size) + create(:lfs_objects_project, project: version.project, lfs_object: lfs_object, repository_type: :design) + end + end + + # This trait is for versions that must be present in the git repository. + trait :committed do + transient do + file { File.join(Rails.root, 'spec/fixtures/dk.png') } + end + + after :create do |version, evaluator| + project = version.issue.project + repository = project.design_repository + repository.create_if_not_exists + + designs = version.designs_by_event + base_change = { content: evaluator.file } + + actions = %w[modification deletion].flat_map { |k| designs.fetch(k, []) }.map do |design| + base_change.merge(action: :create, file_path: design.full_path) + end + + if actions.present? + repository.multi_action( + evaluator.author, + branch_name: 'master', + message: "created #{actions.size} files", + actions: actions + ) + end + + mapping = { + 'creation' => :create, + 'modification' => :update, + 'deletion' => :delete + } + + version_actions = designs.flat_map do |(event, designs)| + base = event == 'deletion' ? {} : base_change + designs.map do |design| + base.merge(action: mapping[event], file_path: design.full_path) + end + end + + sha = repository.multi_action( + evaluator.author, + branch_name: 'master', + message: "edited #{version_actions.size} files", + actions: version_actions + ) + + version.update(sha: sha) + end + end + end +end diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index fdd1a9a18b2..0868e38f70e 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -107,6 +107,10 @@ FactoryBot.define do end end + factory :diff_note_on_design, parent: :note, traits: [:on_design], class: 'DiffNote' do + position { build(:image_diff_position, file: noteable.full_path, diff_refs: noteable.diff_refs) } + end + trait :on_commit do association :project, :repository noteable { nil } @@ -136,6 +140,20 @@ FactoryBot.define do project { nil } end + trait :on_design do + transient do + issue { association(:issue, project: project) } + end + noteable { association(:design, :with_file, issue: issue) } + + after(:build) do |note| + next if note.project == note.noteable.project + + # note validations require consistency between these two objects + note.project = note.noteable.project + end + end + trait :system do system { true } end diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 64321c9f319..45caa7a2b6a 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -215,6 +215,12 @@ FactoryBot.define do end end + trait :design_repo do + after(:create) do |project| + raise 'Failed to create design repository!' unless project.design_repository.create_if_not_exists + end + end + trait :remote_mirror do transient do remote_name { "remote_mirror_#{SecureRandom.hex}" } diff --git a/spec/factories/uploads.rb b/spec/factories/uploads.rb index a060cd7d6f8..b19af277cc3 100644 --- a/spec/factories/uploads.rb +++ b/spec/factories/uploads.rb @@ -65,5 +65,11 @@ FactoryBot.define do model { create(:note) } uploader { "AttachmentUploader" } end + + trait :design_action_image_v432x230_upload do + mount_point { :image_v432x230 } + model { create(:design_action) } + uploader { ::DesignManagement::DesignV432x230Uploader.name } + end end end diff --git a/spec/frontend/alert_management/components/alert_management_list_spec.js b/spec/frontend/alert_management/components/alert_management_list_spec.js index a47363f4dc7..e45a9042f71 100644 --- a/spec/frontend/alert_management/components/alert_management_list_spec.js +++ b/spec/frontend/alert_management/components/alert_management_list_spec.js @@ -1,5 +1,5 @@ import { mount } from '@vue/test-utils'; -import { GlEmptyState, GlTable, GlAlert, GlLoadingIcon } from '@gitlab/ui'; +import { GlEmptyState, GlTable, GlAlert, GlLoadingIcon, GlNewDropdown } from '@gitlab/ui'; import AlertManagementList from '~/alert_management/components/alert_management_list.vue'; import mockAlerts from '../mocks/alerts.json'; @@ -11,6 +11,7 @@ describe('AlertManagementList', () => { const findAlerts = () => wrapper.findAll('table tbody tr'); const findAlert = () => wrapper.find(GlAlert); const findLoader = () => wrapper.find(GlLoadingIcon); + const findStatusDropdown = () => wrapper.find(GlNewDropdown); function mountComponent({ props = { @@ -103,5 +104,14 @@ describe('AlertManagementList', () => { expect(findAlertsTable().exists()).toBe(true); expect(findAlerts()).toHaveLength(mockAlerts.length); }); + + it('displays status dropdown', () => { + mountComponent({ + props: { alertManagementEnabled: true, userCanEnableAlertManagement: true }, + data: { alerts: mockAlerts, errored: false }, + loading: false, + }); + expect(findStatusDropdown().exists()).toBe(true); + }); }); }); diff --git a/spec/lib/gitlab/git_access_design_spec.rb b/spec/lib/gitlab/git_access_design_spec.rb new file mode 100644 index 00000000000..b09afc67c90 --- /dev/null +++ b/spec/lib/gitlab/git_access_design_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Gitlab::GitAccessDesign do + include DesignManagementTestHelpers + + let_it_be(:project) { create(:project) } + let_it_be(:user) { project.owner } + let(:protocol) { 'web' } + let(:actor) { user } + + subject(:access) do + described_class.new(actor, project, protocol, authentication_abilities: [:read_project, :download_code, :push_code]) + end + + describe '#check' do + subject { access.check('git-receive-pack', ::Gitlab::GitAccess::ANY) } + + before do + enable_design_management + end + + context 'when the user is allowed to manage designs' do + # TODO This test is being temporarily skipped unless run in EE, + # as we are in the process of moving Design Management to FOSS in 13.0 + # in steps. In the current step the policies have not yet been moved + # which means that although the `GitAccessDesign` class has moved, the + # user will always be denied access in FOSS. + # + # See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283. + it do + skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? + + is_expected.to be_a(::Gitlab::GitAccessResult::Success) + end + end + + context 'when the user is not allowed to manage designs' do + let_it_be(:user) { create(:user) } + + it 'raises an error' do + expect { subject }.to raise_error(::Gitlab::GitAccess::ForbiddenError) + end + end + + context 'when the protocol is not web' do + let(:protocol) { 'https' } + + it 'raises an error' do + expect { subject }.to raise_error(::Gitlab::GitAccess::ForbiddenError) + end + end + end +end diff --git a/spec/lib/gitlab/gl_repository/repo_type_spec.rb b/spec/lib/gitlab/gl_repository/repo_type_spec.rb index 6185b068d4c..bf6df55b71e 100644 --- a/spec/lib/gitlab/gl_repository/repo_type_spec.rb +++ b/spec/lib/gitlab/gl_repository/repo_type_spec.rb @@ -7,6 +7,7 @@ describe Gitlab::GlRepository::RepoType do let_it_be(:project_snippet) { create(:project_snippet, project: project, author: project.owner) } let(:project_path) { project.repository.full_path } let(:wiki_path) { project.wiki.repository.full_path } + let(:design_path) { project.design_repository.full_path } let(:personal_snippet_path) { "snippets/#{personal_snippet.id}" } let(:project_snippet_path) { "#{project.full_path}/snippets/#{project_snippet.id}" } @@ -24,6 +25,7 @@ describe Gitlab::GlRepository::RepoType do expect(described_class).not_to be_wiki expect(described_class).to be_project expect(described_class).not_to be_snippet + expect(described_class).not_to be_design end end @@ -33,6 +35,7 @@ describe Gitlab::GlRepository::RepoType do expect(described_class.valid?(wiki_path)).to be_truthy expect(described_class.valid?(personal_snippet_path)).to be_truthy expect(described_class.valid?(project_snippet_path)).to be_truthy + expect(described_class.valid?(design_path)).to be_truthy end end end @@ -51,6 +54,7 @@ describe Gitlab::GlRepository::RepoType do expect(described_class).to be_wiki expect(described_class).not_to be_project expect(described_class).not_to be_snippet + expect(described_class).not_to be_design end end @@ -60,6 +64,7 @@ describe Gitlab::GlRepository::RepoType do expect(described_class.valid?(wiki_path)).to be_truthy expect(described_class.valid?(personal_snippet_path)).to be_falsey expect(described_class.valid?(project_snippet_path)).to be_falsey + expect(described_class.valid?(design_path)).to be_falsey end end end @@ -79,6 +84,7 @@ describe Gitlab::GlRepository::RepoType do expect(described_class).to be_snippet expect(described_class).not_to be_wiki expect(described_class).not_to be_project + expect(described_class).not_to be_design end end @@ -88,6 +94,7 @@ describe Gitlab::GlRepository::RepoType do expect(described_class.valid?(wiki_path)).to be_falsey expect(described_class.valid?(personal_snippet_path)).to be_truthy expect(described_class.valid?(project_snippet_path)).to be_truthy + expect(described_class.valid?(design_path)).to be_falsey end end end @@ -115,8 +122,38 @@ describe Gitlab::GlRepository::RepoType do expect(described_class.valid?(wiki_path)).to be_falsey expect(described_class.valid?(personal_snippet_path)).to be_truthy expect(described_class.valid?(project_snippet_path)).to be_truthy + expect(described_class.valid?(design_path)).to be_falsey end end end end + + describe Gitlab::GlRepository::DESIGN do + it_behaves_like 'a repo type' do + let(:expected_identifier) { "design-#{project.id}" } + let(:expected_id) { project.id.to_s } + let(:expected_suffix) { '.design' } + let(:expected_repository) { project.design_repository } + let(:expected_container) { project } + end + + it 'knows its type' do + aggregate_failures do + expect(described_class).to be_design + expect(described_class).not_to be_project + expect(described_class).not_to be_wiki + expect(described_class).not_to be_snippet + end + end + + it 'checks if repository path is valid' do + aggregate_failures do + expect(described_class.valid?(design_path)).to be_truthy + expect(described_class.valid?(project_path)).to be_falsey + expect(described_class.valid?(wiki_path)).to be_falsey + expect(described_class.valid?(personal_snippet_path)).to be_falsey + expect(described_class.valid?(project_snippet_path)).to be_falsey + end + end + end end diff --git a/spec/lib/gitlab/gl_repository_spec.rb b/spec/lib/gitlab/gl_repository_spec.rb index 858f436047e..5f5244b7116 100644 --- a/spec/lib/gitlab/gl_repository_spec.rb +++ b/spec/lib/gitlab/gl_repository_spec.rb @@ -19,6 +19,10 @@ describe ::Gitlab::GlRepository do expect(described_class.parse("snippet-#{snippet.id}")).to eq([snippet, nil, Gitlab::GlRepository::SNIPPET]) end + it 'parses a design gl_repository' do + expect(described_class.parse("design-#{project.id}")).to eq([project, project, Gitlab::GlRepository::DESIGN]) + end + it 'throws an argument error on an invalid gl_repository type' do expect { described_class.parse("badformat-#{project.id}") }.to raise_error(ArgumentError) end @@ -27,4 +31,15 @@ describe ::Gitlab::GlRepository do expect { described_class.parse("project-foo") }.to raise_error(ArgumentError) end end + + describe 'DESIGN' do + it 'uses the design access checker' do + expect(described_class::DESIGN.access_checker_class).to eq(::Gitlab::GitAccessDesign) + end + + it 'builds a design repository' do + expect(described_class::DESIGN.repository_resolver.call(create(:project))) + .to be_a(::DesignManagement::Repository) + end + end end diff --git a/spec/models/design_management/action_spec.rb b/spec/models/design_management/action_spec.rb new file mode 100644 index 00000000000..753c31b1549 --- /dev/null +++ b/spec/models/design_management/action_spec.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DesignManagement::Action do + describe 'relations' do + it { is_expected.to belong_to(:design) } + it { is_expected.to belong_to(:version) } + end + + describe 'scopes' do + describe '.most_recent' do + let_it_be(:design_a) { create(:design) } + let_it_be(:design_b) { create(:design) } + let_it_be(:design_c) { create(:design) } + + let(:designs) { [design_a, design_b, design_c] } + + before_all do + create(:design_version, designs: [design_a, design_b, design_c]) + create(:design_version, designs: [design_a, design_b]) + create(:design_version, designs: [design_a]) + end + + it 'finds the correct version for each design' do + dvs = described_class.where(design: designs) + + expected = designs + .map(&:id) + .zip(dvs.order("version_id DESC").pluck(:version_id).uniq) + + actual = dvs.most_recent.map { |dv| [dv.design_id, dv.version_id] } + + expect(actual).to eq(expected) + end + end + + describe '.up_to_version' do + let_it_be(:issue) { create(:issue) } + let_it_be(:design_a) { create(:design, issue: issue) } + let_it_be(:design_b) { create(:design, issue: issue) } + + # let bindings are not available in before(:all) contexts, + # so we need to redefine the array on each construction. + let_it_be(:oldest) { create(:design_version, designs: [design_a, design_b]) } + let_it_be(:middle) { create(:design_version, designs: [design_a, design_b]) } + let_it_be(:newest) { create(:design_version, designs: [design_a, design_b]) } + + subject { described_class.where(design: issue.designs).up_to_version(version) } + + context 'the version is nil' do + let(:version) { nil } + + it 'returns all design_versions' do + is_expected.to have_attributes(size: 6) + end + end + + context 'when given a Version instance' do + context 'the version is the most current' do + let(:version) { newest } + + it { is_expected.to have_attributes(size: 6) } + end + + context 'the version is the oldest' do + let(:version) { oldest } + + it { is_expected.to have_attributes(size: 2) } + end + + context 'the version is the middle one' do + let(:version) { middle } + + it { is_expected.to have_attributes(size: 4) } + end + end + + context 'when given a commit SHA' do + context 'the version is the most current' do + let(:version) { newest.sha } + + it { is_expected.to have_attributes(size: 6) } + end + + context 'the version is the oldest' do + let(:version) { oldest.sha } + + it { is_expected.to have_attributes(size: 2) } + end + + context 'the version is the middle one' do + let(:version) { middle.sha } + + it { is_expected.to have_attributes(size: 4) } + end + end + + context 'when given a String that is not a commit SHA' do + let(:version) { 'foo' } + + it { expect { subject }.to raise_error(ArgumentError) } + end + end + end +end diff --git a/spec/models/design_management/design_action_spec.rb b/spec/models/design_management/design_action_spec.rb new file mode 100644 index 00000000000..da4ad41dfcb --- /dev/null +++ b/spec/models/design_management/design_action_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DesignManagement::DesignAction do + describe 'validations' do + describe 'the design' do + let(:fail_validation) { raise_error(/design/i) } + + it 'must not be nil' do + expect { described_class.new(nil, :create, :foo) }.to fail_validation + end + end + + describe 'the action' do + let(:fail_validation) { raise_error(/action/i) } + + it 'must not be nil' do + expect { described_class.new(double, nil, :foo) }.to fail_validation + end + + it 'must be a known action' do + expect { described_class.new(double, :wibble, :foo) }.to fail_validation + end + end + + describe 'the content' do + context 'content is necesary' do + let(:fail_validation) { raise_error(/needs content/i) } + + %i[create update].each do |action| + it "must not be nil if the action is #{action}" do + expect { described_class.new(double, action, nil) }.to fail_validation + end + end + end + + context 'content is forbidden' do + let(:fail_validation) { raise_error(/forbids content/i) } + + it "must not be nil if the action is delete" do + expect { described_class.new(double, :delete, :foo) }.to fail_validation + end + end + end + end + + describe '#gitaly_action' do + let(:path) { 'some/path/somewhere' } + let(:design) { OpenStruct.new(full_path: path) } + + subject { described_class.new(design, action, content) } + + context 'the action needs content' do + let(:action) { :create } + let(:content) { :foo } + + it 'produces a good gitaly action' do + expect(subject.gitaly_action).to eq( + action: action, + file_path: path, + content: content + ) + end + end + + context 'the action forbids content' do + let(:action) { :delete } + let(:content) { nil } + + it 'produces a good gitaly action' do + expect(subject.gitaly_action).to eq(action: action, file_path: path) + end + end + end + + describe '#issue_id' do + let(:issue_id) { :foo } + let(:design) { OpenStruct.new(issue_id: issue_id) } + + subject { described_class.new(design, :delete) } + + it 'delegates to the design' do + expect(subject.issue_id).to eq(issue_id) + end + end + + describe '#performed' do + let(:design) { double } + + subject { described_class.new(design, :delete) } + + it 'calls design#clear_version_cache when the action has been performed' do + expect(design).to receive(:clear_version_cache) + + subject.performed + end + end +end diff --git a/spec/models/design_management/design_at_version_spec.rb b/spec/models/design_management/design_at_version_spec.rb new file mode 100644 index 00000000000..f6fa8df243c --- /dev/null +++ b/spec/models/design_management/design_at_version_spec.rb @@ -0,0 +1,426 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DesignManagement::DesignAtVersion do + include DesignManagementTestHelpers + + let_it_be(:issue, reload: true) { create(:issue) } + let_it_be(:issue_b, reload: true) { create(:issue) } + let_it_be(:design, reload: true) { create(:design, issue: issue) } + let_it_be(:version) { create(:design_version, designs: [design]) } + + describe '#id' do + subject { described_class.new(design: design, version: version) } + + it 'combines design.id and version.id' do + expect(subject.id).to include(design.id.to_s, version.id.to_s) + end + end + + describe '#==' do + it 'identifies objects created with the same parameters as equal' do + design = build_stubbed(:design, issue: issue) + version = build_stubbed(:design_version, designs: [design], issue: issue) + + this = build_stubbed(:design_at_version, design: design, version: version) + other = build_stubbed(:design_at_version, design: design, version: version) + + expect(this).to eq(other) + expect(other).to eq(this) + end + + it 'identifies unequal objects as unequal, by virtue of their version' do + design = build_stubbed(:design, issue: issue) + version_a = build_stubbed(:design_version, designs: [design]) + version_b = build_stubbed(:design_version, designs: [design]) + + this = build_stubbed(:design_at_version, design: design, version: version_a) + other = build_stubbed(:design_at_version, design: design, version: version_b) + + expect(this).not_to eq(nil) + expect(this).not_to eq(design) + expect(this).not_to eq(other) + expect(other).not_to eq(this) + end + + it 'identifies unequal objects as unequal, by virtue of their design' do + design_a = build_stubbed(:design, issue: issue) + design_b = build_stubbed(:design, issue: issue) + version = build_stubbed(:design_version, designs: [design_a, design_b]) + + this = build_stubbed(:design_at_version, design: design_a, version: version) + other = build_stubbed(:design_at_version, design: design_b, version: version) + + expect(this).not_to eq(other) + expect(other).not_to eq(this) + end + + it 'rejects objects with the same id and the wrong class' do + dav = build_stubbed(:design_at_version) + + expect(dav).not_to eq(OpenStruct.new(id: dav.id)) + end + + it 'expects objects to be of the same type, not subtypes' do + subtype = Class.new(described_class) + dav = build_stubbed(:design_at_version) + other = subtype.new(design: dav.design, version: dav.version) + + expect(dav).not_to eq(other) + end + end + + describe 'status methods' do + let!(:design_a) { create(:design, issue: issue) } + let!(:design_b) { create(:design, issue: issue) } + + let!(:version_a) do + create(:design_version, designs: [design_a]) + end + let!(:version_b) do + create(:design_version, designs: [design_b]) + end + let!(:version_mod) do + create(:design_version, modified_designs: [design_a, design_b]) + end + let!(:version_c) do + create(:design_version, deleted_designs: [design_a]) + end + let!(:version_d) do + create(:design_version, deleted_designs: [design_b]) + end + let!(:version_e) do + create(:design_version, designs: [design_a]) + end + + describe 'a design before it has been created' do + subject { build(:design_at_version, design: design_b, version: version_a) } + + it 'is not deleted' do + expect(subject).not_to be_deleted + end + + it 'has the status :not_created_yet' do + expect(subject).to have_attributes(status: :not_created_yet) + end + end + + describe 'a design as of its creation' do + subject { build(:design_at_version, design: design_a, version: version_a) } + + it 'is not deleted' do + expect(subject).not_to be_deleted + end + + it 'has the status :current' do + expect(subject).to have_attributes(status: :current) + end + end + + describe 'a design after it has been created, but before deletion' do + subject { build(:design_at_version, design: design_b, version: version_c) } + + it 'is not deleted' do + expect(subject).not_to be_deleted + end + + it 'has the status :current' do + expect(subject).to have_attributes(status: :current) + end + end + + describe 'a design as of its modification' do + subject { build(:design_at_version, design: design_a, version: version_mod) } + + it 'is not deleted' do + expect(subject).not_to be_deleted + end + + it 'has the status :current' do + expect(subject).to have_attributes(status: :current) + end + end + + describe 'a design as of its deletion' do + subject { build(:design_at_version, design: design_a, version: version_c) } + + it 'is deleted' do + expect(subject).to be_deleted + end + + it 'has the status :deleted' do + expect(subject).to have_attributes(status: :deleted) + end + end + + describe 'a design after its deletion' do + subject { build(:design_at_version, design: design_b, version: version_e) } + + it 'is deleted' do + expect(subject).to be_deleted + end + + it 'has the status :deleted' do + expect(subject).to have_attributes(status: :deleted) + end + end + + describe 'a design on its recreation' do + subject { build(:design_at_version, design: design_a, version: version_e) } + + it 'is not deleted' do + expect(subject).not_to be_deleted + end + + it 'has the status :current' do + expect(subject).to have_attributes(status: :current) + end + end + end + + describe 'validations' do + subject(:design_at_version) { build(:design_at_version) } + + it { is_expected.to be_valid } + + describe 'a design-at-version without a design' do + subject { described_class.new(design: nil, version: build(:design_version)) } + + it { is_expected.to be_invalid } + + it 'mentions the design in the errors' do + subject.valid? + + expect(subject.errors[:design]).to be_present + end + end + + describe 'a design-at-version without a version' do + subject { described_class.new(design: build(:design), version: nil) } + + it { is_expected.to be_invalid } + + it 'mentions the version in the errors' do + subject.valid? + + expect(subject.errors[:version]).to be_present + end + end + + describe 'design_and_version_belong_to_the_same_issue' do + context 'both design and version are supplied' do + subject(:design_at_version) { build(:design_at_version, design: design, version: version) } + + context 'the design belongs to the same issue as the version' do + it { is_expected.to be_valid } + end + + context 'the design does not belong to the same issue as the version' do + let(:design) { create(:design) } + let(:version) { create(:design_version) } + + it { is_expected.to be_invalid } + end + end + + context 'the factory is just supplied with a design' do + let(:design) { create(:design) } + + subject(:design_at_version) { build(:design_at_version, design: design) } + + it { is_expected.to be_valid } + end + + context 'the factory is just supplied with a version' do + let(:version) { create(:design_version) } + + subject(:design_at_version) { build(:design_at_version, version: version) } + + it { is_expected.to be_valid } + end + end + + describe 'design_and_version_have_issue_id' do + subject(:design_at_version) { build(:design_at_version, design: design, version: version) } + + context 'the design has no issue_id, because it is being imported' do + let(:design) { create(:design, :importing) } + + it { is_expected.to be_invalid } + end + + context 'the version has no issue_id, because it is being imported' do + let(:version) { create(:design_version, :importing) } + + it { is_expected.to be_invalid } + end + + context 'both the design and the version are being imported' do + let(:version) { create(:design_version, :importing) } + let(:design) { create(:design, :importing) } + + it { is_expected.to be_invalid } + end + end + end + + def id_of(design, version) + build(:design_at_version, design: design, version: version).id + end + + describe '.instantiate' do + context 'when attrs are valid' do + subject do + described_class.instantiate(design: design, version: version) + end + + it { is_expected.to be_a(described_class).and(be_valid) } + end + + context 'when attrs are invalid' do + subject do + described_class.instantiate( + design: create(:design), + version: create(:design_version) + ) + end + + it 'raises a validation error' do + expect { subject }.to raise_error(ActiveModel::ValidationError) + end + end + end + + describe '.lazy_find' do + let!(:version_a) do + create(:design_version, designs: create_list(:design, 3, issue: issue)) + end + let!(:version_b) do + create(:design_version, designs: create_list(:design, 1, issue: issue)) + end + let!(:version_c) do + create(:design_version, designs: create_list(:design, 1, issue: issue_b)) + end + + let(:id_a) { id_of(version_a.designs.first, version_a) } + let(:id_b) { id_of(version_a.designs.second, version_a) } + let(:id_c) { id_of(version_a.designs.last, version_a) } + let(:id_d) { id_of(version_b.designs.first, version_b) } + let(:id_e) { id_of(version_c.designs.first, version_c) } + let(:bad_id) { id_of(version_c.designs.first, version_a) } + + def find(the_id) + described_class.lazy_find(the_id) + end + + let(:db_calls) { 2 } + + it 'issues fewer queries than the naive approach would' do + expect do + dav_a = find(id_a) + dav_b = find(id_b) + dav_c = find(id_c) + dav_d = find(id_d) + dav_e = find(id_e) + should_not_exist = find(bad_id) + + expect(dav_a.version).to eq(version_a) + expect(dav_b.version).to eq(version_a) + expect(dav_c.version).to eq(version_a) + expect(dav_d.version).to eq(version_b) + expect(dav_e.version).to eq(version_c) + expect(should_not_exist).not_to be_present + + expect(version_a.designs).to include(dav_a.design, dav_b.design, dav_c.design) + expect(version_b.designs).to include(dav_d.design) + expect(version_c.designs).to include(dav_e.design) + end.not_to exceed_query_limit(db_calls) + end + end + + describe '.find' do + let(:results) { described_class.find(ids) } + + # 2 versions, with 5 total designs on issue A, so 2*5 = 10 + let!(:version_a) do + create(:design_version, designs: create_list(:design, 3, issue: issue)) + end + let!(:version_b) do + create(:design_version, designs: create_list(:design, 2, issue: issue)) + end + # 1 version, with 3 designs on issue B, so 1*3 = 3 + let!(:version_c) do + create(:design_version, designs: create_list(:design, 3, issue: issue_b)) + end + + context 'invalid ids' do + let(:ids) do + version_b.designs.map { |d| id_of(d, version_c) } + end + + describe '#count' do + it 'counts 0 records' do + expect(results.count).to eq(0) + end + end + + describe '#empty?' do + it 'is empty' do + expect(results).to be_empty + end + end + + describe '#to_a' do + it 'finds no records' do + expect(results.to_a).to eq([]) + end + end + end + + context 'valid ids' do + let(:red_herrings) { issue_b.designs.sample(2).map { |d| id_of(d, version_a) } } + + let(:ids) do + a_ids = issue.designs.sample(2).map { |d| id_of(d, version_a) } + b_ids = issue.designs.sample(2).map { |d| id_of(d, version_b) } + c_ids = issue_b.designs.sample(2).map { |d| id_of(d, version_c) } + + a_ids + b_ids + c_ids + red_herrings + end + + before do + ids.size # force IDs + end + + describe '#count' do + it 'counts 2 records' do + expect(results.count).to eq(6) + end + + it 'issues at most two queries' do + expect { results.count }.not_to exceed_query_limit(2) + end + end + + describe '#to_a' do + it 'finds 6 records' do + expect(results.size).to eq(6) + expect(results).to all(be_a(described_class)) + end + + it 'only returns records with matching IDs' do + expect(results.map(&:id)).to match_array(ids - red_herrings) + end + + it 'only returns valid records' do + expect(results).to all(be_valid) + end + + it 'issues at most two queries' do + expect { results.to_a }.not_to exceed_query_limit(2) + end + end + end + end +end diff --git a/spec/models/design_management/design_collection_spec.rb b/spec/models/design_management/design_collection_spec.rb new file mode 100644 index 00000000000..bd48f742042 --- /dev/null +++ b/spec/models/design_management/design_collection_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DesignManagement::DesignCollection do + include DesignManagementTestHelpers + + let_it_be(:issue, reload: true) { create(:issue) } + + subject(:collection) { described_class.new(issue) } + + describe ".find_or_create_design!" do + it "finds an existing design" do + design = create(:design, issue: issue, filename: 'world.png') + + expect(collection.find_or_create_design!(filename: 'world.png')).to eq(design) + end + + it "creates a new design if one didn't exist" do + expect(issue.designs.size).to eq(0) + + new_design = collection.find_or_create_design!(filename: 'world.png') + + expect(issue.designs.size).to eq(1) + expect(new_design.filename).to eq('world.png') + expect(new_design.issue).to eq(issue) + end + + it "only queries the designs once" do + create(:design, issue: issue, filename: 'hello.png') + create(:design, issue: issue, filename: 'world.jpg') + + expect do + collection.find_or_create_design!(filename: 'hello.png') + collection.find_or_create_design!(filename: 'world.jpg') + end.not_to exceed_query_limit(1) + end + end + + describe "#versions" do + it "includes versions for all designs" do + version_1 = create(:design_version) + version_2 = create(:design_version) + other_version = create(:design_version) + create(:design, issue: issue, versions: [version_1]) + create(:design, issue: issue, versions: [version_2]) + create(:design, versions: [other_version]) + + expect(collection.versions).to contain_exactly(version_1, version_2) + end + end + + describe "#repository" do + it "builds a design repository" do + expect(collection.repository).to be_a(DesignManagement::Repository) + end + end + + describe '#designs_by_filename' do + let(:designs) { create_list(:design, 5, :with_file, issue: issue) } + let(:filenames) { designs.map(&:filename) } + let(:query) { subject.designs_by_filename(filenames) } + + it 'finds all the designs with those filenames on this issue' do + expect(query).to have_attributes(size: 5) + end + + it 'only makes a single query' do + designs.each(&:id) + expect { query }.not_to exceed_query_limit(1) + end + + context 'some are deleted' do + before do + delete_designs(*designs.sample(2)) + end + + it 'takes deletion into account' do + expect(query).to have_attributes(size: 3) + end + end + end +end diff --git a/spec/models/design_management/design_spec.rb b/spec/models/design_management/design_spec.rb new file mode 100644 index 00000000000..555b7f04f86 --- /dev/null +++ b/spec/models/design_management/design_spec.rb @@ -0,0 +1,582 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DesignManagement::Design do + include DesignManagementTestHelpers + + let_it_be(:issue) { create(:issue) } + let_it_be(:design1) { create(:design, :with_versions, issue: issue, versions_count: 1) } + let_it_be(:design2) { create(:design, :with_versions, issue: issue, versions_count: 1) } + let_it_be(:design3) { create(:design, :with_versions, issue: issue, versions_count: 1) } + let_it_be(:deleted_design) { create(:design, :with_versions, deleted: true) } + + describe 'relations' do + it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:issue) } + it { is_expected.to have_many(:actions) } + it { is_expected.to have_many(:versions) } + it { is_expected.to have_many(:notes).dependent(:delete_all) } + it { is_expected.to have_many(:user_mentions) } + end + + describe 'validations' do + subject(:design) { build(:design) } + + it { is_expected.to be_valid } + it { is_expected.to validate_presence_of(:project) } + it { is_expected.to validate_presence_of(:issue) } + it { is_expected.to validate_presence_of(:filename) } + it { is_expected.to validate_uniqueness_of(:filename).scoped_to(:issue_id) } + + it "validates that the extension is an image" do + design.filename = "thing.txt" + extensions = described_class::SAFE_IMAGE_EXT + described_class::DANGEROUS_IMAGE_EXT + + expect(design).not_to be_valid + expect(design.errors[:filename].first).to eq( + "does not have a supported extension. Only #{extensions.to_sentence} are supported" + ) + end + + describe 'validating files with .svg extension' do + before do + design.filename = "thing.svg" + end + + it "allows .svg files when feature flag is enabled" do + stub_feature_flags(design_management_allow_dangerous_images: true) + + expect(design).to be_valid + end + + it "does not allow .svg files when feature flag is disabled" do + stub_feature_flags(design_management_allow_dangerous_images: false) + + expect(design).not_to be_valid + expect(design.errors[:filename].first).to eq( + "does not have a supported extension. Only #{described_class::SAFE_IMAGE_EXT.to_sentence} are supported" + ) + end + end + end + + describe 'scopes' do + describe '.visible_at_version' do + let(:versions) { DesignManagement::Version.where(issue: issue).ordered } + let(:found) { described_class.visible_at_version(version) } + + context 'at oldest version' do + let(:version) { versions.last } + + it 'finds the first design only' do + expect(found).to contain_exactly(design1) + end + end + + context 'at version 2' do + let(:version) { versions.second } + + it 'finds the first and second designs' do + expect(found).to contain_exactly(design1, design2) + end + end + + context 'at latest version' do + let(:version) { versions.first } + + it 'finds designs' do + expect(found).to contain_exactly(design1, design2, design3) + end + end + + context 'when the argument is nil' do + let(:version) { nil } + + it 'finds all undeleted designs' do + expect(found).to contain_exactly(design1, design2, design3) + end + end + + describe 'one of the designs was deleted before the given version' do + before do + delete_designs(design2) + end + + it 'is not returned' do + current_version = versions.first + + expect(described_class.visible_at_version(current_version)).to contain_exactly(design1, design3) + end + end + + context 'a re-created history' do + before do + delete_designs(design1, design2) + restore_designs(design1) + end + + it 'is returned, though other deleted events are not' do + expect(described_class.visible_at_version(nil)).to contain_exactly(design1, design3) + end + end + + # test that a design that has been modified at various points + # can be queried for correctly at different points in its history + describe 'dead or alive' do + let(:versions) { DesignManagement::Version.where(issue: issue).map { |v| [v, :alive] } } + + before do + versions << [delete_designs(design1), :dead] + versions << [modify_designs(design2), :dead] + versions << [restore_designs(design1), :alive] + versions << [modify_designs(design3), :alive] + versions << [delete_designs(design1), :dead] + versions << [modify_designs(design2, design3), :dead] + versions << [restore_designs(design1), :alive] + end + + it 'can establish the history at any point' do + history = versions.map(&:first).map do |v| + described_class.visible_at_version(v).include?(design1) ? :alive : :dead + end + + expect(history).to eq(versions.map(&:second)) + end + end + end + + describe '.with_filename' do + it 'returns correct design when passed a single filename' do + expect(described_class.with_filename(design1.filename)).to eq([design1]) + end + + it 'returns correct designs when passed an Array of filenames' do + expect( + described_class.with_filename([design1, design2].map(&:filename)) + ).to contain_exactly(design1, design2) + end + end + + describe '.on_issue' do + it 'returns correct designs when passed a single issue' do + expect(described_class.on_issue(issue)).to match_array(issue.designs) + end + + it 'returns correct designs when passed an Array of issues' do + expect( + described_class.on_issue([issue, deleted_design.issue]) + ).to contain_exactly(design1, design2, design3, deleted_design) + end + end + + describe '.current' do + it 'returns just the undeleted designs' do + delete_designs(design3) + + expect(described_class.current).to contain_exactly(design1, design2) + end + end + end + + describe '#visible_in?' do + let_it_be(:issue) { create(:issue) } + + # It is expensive to re-create complex histories, so we do it once, and then + # assert that we can establish visibility at any given version. + it 'tells us when a design is visible' do + expected = [] + + first_design = create(:design, :with_versions, issue: issue, versions_count: 1) + prior_to_creation = first_design.versions.first + expected << [prior_to_creation, :not_created_yet, false] + + v = modify_designs(first_design) + expected << [v, :not_created_yet, false] + + design = create(:design, :with_versions, issue: issue, versions_count: 1) + created_in = design.versions.first + expected << [created_in, :created, true] + + # The future state should not affect the result for any state, so we + # ensure that most states have a long future as well as a rich past + 2.times do + v = modify_designs(first_design) + expected << [v, :unaffected_visible, true] + + v = modify_designs(design) + expected << [v, :modified, true] + + v = modify_designs(first_design) + expected << [v, :unaffected_visible, true] + + v = delete_designs(design) + expected << [v, :deleted, false] + + v = modify_designs(first_design) + expected << [v, :unaffected_nv, false] + + v = restore_designs(design) + expected << [v, :restored, true] + end + + delete_designs(design) # ensure visibility is not corelated with current state + + got = expected.map do |(v, sym, _)| + [v, sym, design.visible_in?(v)] + end + + expect(got).to eq(expected) + end + end + + describe '#to_ability_name' do + it { expect(described_class.new.to_ability_name).to eq('design') } + end + + describe '#status' do + context 'the design is new' do + subject { build(:design) } + + it { is_expected.to have_attributes(status: :new) } + end + + context 'the design is current' do + subject { design1 } + + it { is_expected.to have_attributes(status: :current) } + end + + context 'the design has been deleted' do + subject { deleted_design } + + it { is_expected.to have_attributes(status: :deleted) } + end + end + + describe '#deleted?' do + context 'the design is new' do + let(:design) { build(:design) } + + it 'is falsy' do + expect(design).not_to be_deleted + end + end + + context 'the design is current' do + let(:design) { design1 } + + it 'is falsy' do + expect(design).not_to be_deleted + end + end + + context 'the design has been deleted' do + let(:design) { deleted_design } + + it 'is truthy' do + expect(design).to be_deleted + end + end + + context 'the design has been deleted, but was then re-created' do + let(:design) { create(:design, :with_versions, versions_count: 1, deleted: true) } + + it 'is falsy' do + restore_designs(design) + + expect(design).not_to be_deleted + end + end + end + + describe "#new_design?" do + let(:design) { design1 } + + it "is false when there are versions" do + expect(design1).not_to be_new_design + end + + it "is true when there are no versions" do + expect(build(:design)).to be_new_design + end + + it 'is false for deleted designs' do + expect(deleted_design).not_to be_new_design + end + + it "does not cause extra queries when actions are loaded" do + design.actions.map(&:id) + + expect { design.new_design? }.not_to exceed_query_limit(0) + end + + it "implicitly caches values" do + expect do + design.new_design? + design.new_design? + end.not_to exceed_query_limit(1) + end + + it "queries again when the clear_version_cache trigger has been called" do + expect do + design.new_design? + design.clear_version_cache + design.new_design? + end.not_to exceed_query_limit(2) + end + + it "causes a single query when there versions are not loaded" do + design.reload + + expect { design.new_design? }.not_to exceed_query_limit(1) + end + end + + describe "#full_path" do + it "builds the full path for a design" do + design = build(:design, filename: "hello.jpg") + expected_path = "#{DesignManagement.designs_directory}/issue-#{design.issue.iid}/hello.jpg" + + expect(design.full_path).to eq(expected_path) + end + end + + describe '#diff_refs' do + let(:design) { create(:design, :with_file, versions_count: versions_count) } + + context 'there are several versions' do + let(:versions_count) { 3 } + + it "builds diff refs based on the first commit and it's for the design" do + expect(design.diff_refs.base_sha).to eq(design.versions.ordered.second.sha) + expect(design.diff_refs.head_sha).to eq(design.versions.ordered.first.sha) + end + end + + context 'there is just one version' do + let(:versions_count) { 1 } + + it 'builds diff refs based on the empty tree if there was only one version' do + design = create(:design, :with_file, versions_count: 1) + + expect(design.diff_refs.base_sha).to eq(Gitlab::Git::BLANK_SHA) + expect(design.diff_refs.head_sha).to eq(design.diff_refs.head_sha) + end + end + + it 'has no diff ref if new' do + design = build(:design) + + expect(design.diff_refs).to be_nil + end + end + + describe '#repository' do + it 'is a design repository' do + design = build(:design) + + expect(design.repository).to be_a(DesignManagement::Repository) + end + end + + # TODO these tests are being temporarily skipped unless run in EE, + # as we are in the process of moving Design Management to FOSS in 13.0 + # in steps. In the current step the routes have not yet been moved. + # + # See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283. + describe '#note_etag_key' do + it 'returns a correct etag key' do + skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? + + design = create(:design) + + expect(design.note_etag_key).to eq( + ::Gitlab::Routing.url_helpers.designs_project_issue_path(design.project, design.issue, { vueroute: design.filename }) + ) + end + end + + describe '#user_notes_count', :use_clean_rails_memory_store_caching do + let_it_be(:design) { create(:design, :with_file) } + + subject { design.user_notes_count } + + # Note: Cache invalidation tests are in `design_user_notes_count_service_spec.rb` + + it 'returns a count of user-generated notes' do + create(:diff_note_on_design, noteable: design) + + is_expected.to eq(1) + end + + it 'does not count notes on other designs' do + second_design = create(:design, :with_file) + create(:diff_note_on_design, noteable: second_design) + + is_expected.to eq(0) + end + + it 'does not count system notes' do + create(:diff_note_on_design, system: true, noteable: design) + + is_expected.to eq(0) + end + end + + describe '#after_note_changed' do + subject { build(:design) } + + it 'calls #delete_cache on DesignUserNotesCountService' do + expect_next_instance_of(DesignManagement::DesignUserNotesCountService) do |service| + expect(service).to receive(:delete_cache) + end + + subject.after_note_changed(build(:note)) + end + + it 'does not call #delete_cache on DesignUserNotesCountService when passed a system note' do + expect(DesignManagement::DesignUserNotesCountService).not_to receive(:new) + + subject.after_note_changed(build(:note, :system)) + end + end + + describe '.for_reference' do + let_it_be(:design_a) { create(:design) } + let_it_be(:design_b) { create(:design) } + + it 'avoids extra queries when calling to_reference' do + designs = described_class.for_reference.where(id: [design_a.id, design_b.id]).to_a + + expect { designs.map(&:to_reference) }.not_to exceed_query_limit(0) + end + end + + describe '#to_reference' do + let(:namespace) { build(:namespace, path: 'sample-namespace') } + let(:project) { build(:project, name: 'sample-project', namespace: namespace) } + let(:group) { create(:group, name: 'Group', path: 'sample-group') } + let(:issue) { build(:issue, iid: 1, project: project) } + let(:filename) { 'homescreen.jpg' } + let(:design) { build(:design, filename: filename, issue: issue, project: project) } + + context 'when nil argument' do + let(:reference) { design.to_reference } + + it 'uses the simple format' do + expect(reference).to eq "#1[homescreen.jpg]" + end + + context 'when the filename contains spaces, hyphens, periods, single-quotes, underscores and colons' do + let(:filename) { %q{a complex filename: containing - _ : etc., but still 'simple'.gif} } + + it 'uses the simple format' do + expect(reference).to eq "#1[#{filename}]" + end + end + + context 'when the filename contains HTML angle brackets' do + let(:filename) { 'a <em>great</em> filename.jpg' } + + it 'uses Base64 encoding' do + expect(reference).to eq "#1[base64:#{Base64.strict_encode64(filename)}]" + end + end + + context 'when the filename contains quotation marks' do + let(:filename) { %q{a "great" filename.jpg} } + + it 'uses enclosing quotes, with backslash encoding' do + expect(reference).to eq %q{#1["a \"great\" filename.jpg"]} + end + end + + context 'when the filename contains square brackets' do + let(:filename) { %q{a [great] filename.jpg} } + + it 'uses enclosing quotes' do + expect(reference).to eq %q{#1["a [great] filename.jpg"]} + end + end + end + + context 'when full is true' do + it 'returns complete path to the issue' do + refs = [ + design.to_reference(full: true), + design.to_reference(project, full: true), + design.to_reference(group, full: true) + ] + + expect(refs).to all(eq 'sample-namespace/sample-project#1/designs[homescreen.jpg]') + end + end + + context 'when full is false' do + it 'returns complete path to the issue' do + refs = [ + design.to_reference(build(:project), full: false), + design.to_reference(group, full: false) + ] + + expect(refs).to all(eq 'sample-namespace/sample-project#1[homescreen.jpg]') + end + end + + context 'when same project argument' do + it 'returns bare reference' do + expect(design.to_reference(project)).to eq("#1[homescreen.jpg]") + end + end + end + + describe 'reference_pattern' do + let(:match) { described_class.reference_pattern.match(ref) } + let(:ref) { design.to_reference } + let(:design) { build(:design, filename: filename) } + + context 'simple_file_name' do + let(:filename) { 'simple-file-name.jpg' } + + it 'matches :simple_file_name' do + expect(match[:simple_file_name]).to eq(filename) + end + end + + context 'quoted_file_name' do + let(:filename) { 'simple "file" name.jpg' } + + it 'matches :simple_file_name' do + expect(match[:escaped_filename].gsub(/\\"/, '"')).to eq(filename) + end + end + + context 'Base64 name' do + let(:filename) { '<>.png' } + + it 'matches base_64_encoded_name' do + expect(Base64.decode64(match[:base_64_encoded_name])).to eq(filename) + end + end + end + + describe '.by_issue_id_and_filename' do + let_it_be(:issue_a) { create(:issue) } + let_it_be(:issue_b) { create(:issue) } + + let_it_be(:design_a) { create(:design, issue: issue_a) } + let_it_be(:design_b) { create(:design, issue: issue_a) } + let_it_be(:design_c) { create(:design, issue: issue_b, filename: design_a.filename) } + let_it_be(:design_d) { create(:design, issue: issue_b, filename: design_b.filename) } + + it_behaves_like 'a where_composite scope', :by_issue_id_and_filename do + let(:all_results) { [design_a, design_b, design_c, design_d] } + let(:first_result) { design_a } + + let(:composite_ids) do + all_results.map { |design| { issue_id: design.issue_id, filename: design.filename } } + end + end + end +end diff --git a/spec/models/design_management/repository_spec.rb b/spec/models/design_management/repository_spec.rb new file mode 100644 index 00000000000..996316eeec9 --- /dev/null +++ b/spec/models/design_management/repository_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DesignManagement::Repository do + let(:project) { create(:project) } + let(:repository) { described_class.new(project) } + + shared_examples 'returns parsed git attributes that enable LFS for all file types' do + it do + expect(subject.patterns).to be_a_kind_of(Hash) + expect(subject.patterns).to have_key('/designs/*') + expect(subject.patterns['/designs/*']).to eql( + { "filter" => "lfs", "diff" => "lfs", "merge" => "lfs", "text" => false } + ) + end + end + + describe "#info_attributes" do + subject { repository.info_attributes } + + include_examples 'returns parsed git attributes that enable LFS for all file types' + end + + describe '#attributes_at' do + subject { repository.attributes_at } + + include_examples 'returns parsed git attributes that enable LFS for all file types' + end + + describe '#gitattribute' do + it 'returns a gitattribute when path has gitattributes' do + expect(repository.gitattribute('/designs/file.txt', 'filter')).to eq('lfs') + end + + it 'returns nil when path has no gitattributes' do + expect(repository.gitattribute('/invalid/file.txt', 'filter')).to be_nil + end + end + + describe '#copy_gitattributes' do + it 'always returns regardless of whether given a valid or invalid ref' do + expect(repository.copy_gitattributes('master')).to be true + expect(repository.copy_gitattributes('invalid')).to be true + end + end + + describe '#attributes' do + it 'confirms that all files are LFS enabled' do + %w(png zip anything).each do |filetype| + path = "/#{DesignManagement.designs_directory}/file.#{filetype}" + attributes = repository.attributes(path) + + expect(attributes['filter']).to eq('lfs') + end + end + end +end diff --git a/spec/models/design_management/version_spec.rb b/spec/models/design_management/version_spec.rb new file mode 100644 index 00000000000..ab6958ea94a --- /dev/null +++ b/spec/models/design_management/version_spec.rb @@ -0,0 +1,342 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DesignManagement::Version do + let_it_be(:issue) { create(:issue) } + + describe 'relations' do + it { is_expected.to have_many(:actions) } + it { is_expected.to have_many(:designs).through(:actions) } + + it 'constrains the designs relation correctly' do + design = create(:design) + version = create(:design_version, designs: [design]) + + expect { version.designs << design }.to raise_error(ActiveRecord::RecordNotUnique) + end + + it 'allows adding multiple versions to a single design' do + design = create(:design) + versions = create_list(:design_version, 2) + + expect { versions.each { |v| design.versions << v } } + .not_to raise_error + end + end + + describe 'validations' do + subject(:design_version) { build(:design_version) } + + it { is_expected.to be_valid } + it { is_expected.to validate_presence_of(:author) } + it { is_expected.to validate_presence_of(:sha) } + it { is_expected.to validate_presence_of(:designs) } + it { is_expected.to validate_presence_of(:issue_id) } + it { is_expected.to validate_uniqueness_of(:sha).scoped_to(:issue_id).case_insensitive } + end + + describe "scopes" do + let_it_be(:version_1) { create(:design_version) } + let_it_be(:version_2) { create(:design_version) } + + describe ".for_designs" do + it "only returns versions related to the specified designs" do + _other_version = create(:design_version) + designs = [create(:design, versions: [version_1]), + create(:design, versions: [version_2])] + + expect(described_class.for_designs(designs)) + .to contain_exactly(version_1, version_2) + end + end + + describe '.earlier_or_equal_to' do + it 'only returns versions created earlier or later than the given version' do + expect(described_class.earlier_or_equal_to(version_1)).to eq([version_1]) + expect(described_class.earlier_or_equal_to(version_2)).to contain_exactly(version_1, version_2) + end + + it 'can be passed either a DesignManagement::Version or an ID' do + [version_1, version_1.id].each do |arg| + expect(described_class.earlier_or_equal_to(arg)).to eq([version_1]) + end + end + end + + describe '.by_sha' do + it 'can find versions by sha' do + [version_1, version_2].each do |version| + expect(described_class.by_sha(version.sha)).to contain_exactly(version) + end + end + end + end + + describe ".create_for_designs" do + def current_version_id(design) + design.send(:head_version).try(:id) + end + + def as_actions(designs, action = :create) + designs.map do |d| + DesignManagement::DesignAction.new(d, action, action == :delete ? nil : :content) + end + end + + let_it_be(:author) { create(:user) } + let_it_be(:design_a) { create(:design, issue: issue) } + let_it_be(:design_b) { create(:design, issue: issue) } + let_it_be(:designs) { [design_a, design_b] } + + describe 'the error raised when there are no actions' do + let_it_be(:sha) { 'f00' } + + def call_with_empty_actions + described_class.create_for_designs([], sha, author) + end + + it 'raises CouldNotCreateVersion' do + expect { call_with_empty_actions } + .to raise_error(described_class::CouldNotCreateVersion) + end + + it 'has an appropriate cause' do + expect { call_with_empty_actions } + .to raise_error(have_attributes(cause: ActiveRecord::RecordInvalid)) + end + + it 'provides extra data sentry can consume' do + extra_info = a_hash_including(sha: sha) + + expect { call_with_empty_actions } + .to raise_error(have_attributes(sentry_extra_data: extra_info)) + end + end + + describe 'the error raised when the designs come from different issues' do + let_it_be(:sha) { 'f00' } + let_it_be(:designs) { create_list(:design, 2) } + let_it_be(:actions) { as_actions(designs) } + + def call_with_mismatched_designs + described_class.create_for_designs(actions, sha, author) + end + + it 'raises CouldNotCreateVersion' do + expect { call_with_mismatched_designs } + .to raise_error(described_class::CouldNotCreateVersion) + end + + it 'has an appropriate cause' do + expect { call_with_mismatched_designs } + .to raise_error(have_attributes(cause: described_class::NotSameIssue)) + end + + it 'provides extra data sentry can consume' do + extra_info = a_hash_including(design_ids: designs.map(&:id)) + + expect { call_with_mismatched_designs } + .to raise_error(have_attributes(sentry_extra_data: extra_info)) + end + end + + it 'does not leave invalid versions around if creation fails' do + expect do + described_class.create_for_designs([], 'abcdef', author) rescue nil + end.not_to change { described_class.count } + end + + it 'does not leave orphaned design-versions around if creation fails' do + actions = as_actions(designs) + expect do + described_class.create_for_designs(actions, '', author) rescue nil + end.not_to change { DesignManagement::Action.count } + end + + it 'creates a version and links it to multiple designs' do + actions = as_actions(designs, :create) + + version = described_class.create_for_designs(actions, 'abc', author) + + expect(version.designs).to contain_exactly(*designs) + expect(designs.map(&method(:current_version_id))).to all(eq version.id) + end + + it 'creates designs if they are new to git' do + actions = as_actions(designs, :create) + + described_class.create_for_designs(actions, 'abc', author) + + expect(designs.map(&:most_recent_action)).to all(be_creation) + end + + it 'correctly associates the version with the issue' do + actions = as_actions(designs) + + version = described_class.create_for_designs(actions, 'abc', author) + + expect(version.issue).to eq(issue) + end + + it 'correctly associates the version with the author' do + actions = as_actions(designs) + + version = described_class.create_for_designs(actions, 'abc', author) + + expect(version.author).to eq(author) + end + + it 'modifies designs if git updated them' do + actions = as_actions(designs, :update) + + described_class.create_for_designs(actions, 'abc', author) + + expect(designs.map(&:most_recent_action)).to all(be_modification) + end + + it 'deletes designs when the git action was delete' do + actions = as_actions(designs, :delete) + + described_class.create_for_designs(actions, 'def', author) + + expect(designs).to all(be_deleted) + end + + it 're-creates designs if they are deleted' do + described_class.create_for_designs(as_actions(designs, :create), 'abc', author) + described_class.create_for_designs(as_actions(designs, :delete), 'def', author) + + expect(designs).to all(be_deleted) + + described_class.create_for_designs(as_actions(designs, :create), 'ghi', author) + + expect(designs.map(&:most_recent_action)).to all(be_creation) + expect(designs).not_to include(be_deleted) + end + + it 'changes the version of the designs' do + actions = as_actions([design_a]) + described_class.create_for_designs(actions, 'before', author) + + expect do + described_class.create_for_designs(actions, 'after', author) + end.to change { current_version_id(design_a) } + end + end + + describe '#designs_by_event' do + context 'there is a single design' do + let_it_be(:design) { create(:design) } + + shared_examples :a_correctly_categorised_design do |kind, category| + let_it_be(:version) { create(:design_version, kind => [design]) } + + it 'returns a hash with a single key and the single design in that bucket' do + expect(version.designs_by_event).to eq(category => [design]) + end + end + + it_behaves_like :a_correctly_categorised_design, :created_designs, 'creation' + it_behaves_like :a_correctly_categorised_design, :modified_designs, 'modification' + it_behaves_like :a_correctly_categorised_design, :deleted_designs, 'deletion' + end + + context 'there are a bunch of different designs in a variety of states' do + let_it_be(:version) do + create(:design_version, + created_designs: create_list(:design, 3), + modified_designs: create_list(:design, 4), + deleted_designs: create_list(:design, 5)) + end + + it 'puts them in the right buckets' do + expect(version.designs_by_event).to match( + a_hash_including( + 'creation' => have_attributes(size: 3), + 'modification' => have_attributes(size: 4), + 'deletion' => have_attributes(size: 5) + ) + ) + end + + it 'does not suffer from N+1 queries' do + version.designs.map(&:id) # we don't care about the set-up queries + expect { version.designs_by_event }.not_to exceed_query_limit(2) + end + end + end + + describe '#author' do + it 'returns the author' do + author = build(:user) + version = build(:design_version, author: author) + + expect(version.author).to eq(author) + end + + it 'returns nil if author_id is nil and version is not persisted' do + version = build(:design_version, author: nil) + + expect(version.author).to eq(nil) + end + + it 'retrieves author from the Commit if author_id is nil and version has been persisted' do + author = create(:user) + version = create(:design_version, :committed, author: author) + author.destroy + version.reload + commit = version.issue.project.design_repository.commit(version.sha) + commit_user = create(:user, email: commit.author_email, name: commit.author_name) + + expect(version.author_id).to eq(nil) + expect(version.author).to eq(commit_user) + end + end + + describe '#diff_refs' do + let(:project) { issue.project } + + before do + expect(project.design_repository).to receive(:commit) + .once + .with(sha) + .and_return(commit) + end + + subject { create(:design_version, issue: issue, sha: sha) } + + context 'there is a commit in the repo by the SHA' do + let(:commit) { build(:commit) } + let(:sha) { commit.id } + + it { is_expected.to have_attributes(diff_refs: commit.diff_refs) } + + it 'memoizes calls to #diff_refs' do + expect(subject.diff_refs).to eq(subject.diff_refs) + end + end + + context 'there is no commit in the repo by the SHA' do + let(:commit) { nil } + let(:sha) { Digest::SHA1.hexdigest("points to nothing") } + + it { is_expected.to have_attributes(diff_refs: be_nil) } + end + end + + describe '#reset' do + subject { create(:design_version, issue: issue) } + + it 'removes memoized values' do + expect(subject).to receive(:commit).twice.and_return(nil) + + subject.diff_refs + subject.diff_refs + + subject.reset + + subject.diff_refs + subject.diff_refs + end + end +end diff --git a/spec/models/design_user_mention_spec.rb b/spec/models/design_user_mention_spec.rb new file mode 100644 index 00000000000..03c77c73c8d --- /dev/null +++ b/spec/models/design_user_mention_spec.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DesignUserMention do + describe 'associations' do + it { is_expected.to belong_to(:design) } + it { is_expected.to belong_to(:note) } + end + + it_behaves_like 'has user mentions' +end diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index b802c8ba506..65f06a5b270 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -287,6 +287,24 @@ describe DiffNote do reply_diff_note.reload.diff_file end end + + context 'when noteable is a Design' do + it 'does not return a diff file' do + diff_note = create(:diff_note_on_design) + + expect(diff_note.diff_file).to be_nil + end + end + end + + describe '#latest_diff_file' do + context 'when noteable is a Design' do + it 'does not return a diff file' do + diff_note = create(:diff_note_on_design) + + expect(diff_note.latest_diff_file).to be_nil + end + end end describe "#diff_line" do diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 375c8c1546f..cc7dffb93d2 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -15,8 +15,20 @@ describe Issue do it { is_expected.to belong_to(:closed_by).class_name('User') } it { is_expected.to have_many(:assignees) } it { is_expected.to have_many(:user_mentions).class_name("IssueUserMention") } + it { is_expected.to have_many(:designs) } + it { is_expected.to have_many(:design_versions) } it { is_expected.to have_one(:sentry_issue) } it { is_expected.to have_one(:alert_management_alert) } + + describe 'versions.most_recent' do + it 'returns the most recent version' do + issue = create(:issue) + create_list(:design_version, 2, issue: issue) + last_version = create(:design_version, issue: issue) + + expect(issue.design_versions.most_recent).to eq(last_version) + end + end end describe 'modules' do @@ -970,4 +982,48 @@ describe Issue do expect(issue.previous_updated_at).to eq(Time.new(2013, 02, 06)) end end + + describe '#design_collection' do + it 'returns a design collection' do + issue = build(:issue) + collection = issue.design_collection + + expect(collection).to be_a(DesignManagement::DesignCollection) + expect(collection.issue).to eq(issue) + end + end + + describe 'current designs' do + let(:issue) { create(:issue) } + + subject { issue.designs.current } + + context 'an issue has no designs' do + it { is_expected.to be_empty } + end + + context 'an issue only has current designs' do + let!(:design_a) { create(:design, :with_file, issue: issue) } + let!(:design_b) { create(:design, :with_file, issue: issue) } + let!(:design_c) { create(:design, :with_file, issue: issue) } + + it { is_expected.to include(design_a, design_b, design_c) } + end + + context 'an issue only has deleted designs' do + let!(:design_a) { create(:design, :with_file, issue: issue, deleted: true) } + let!(:design_b) { create(:design, :with_file, issue: issue, deleted: true) } + let!(:design_c) { create(:design, :with_file, issue: issue, deleted: true) } + + it { is_expected.to be_empty } + end + + context 'an issue has a mixture of current and deleted designs' do + let!(:design_a) { create(:design, :with_file, issue: issue) } + let!(:design_b) { create(:design, :with_file, issue: issue, deleted: true) } + let!(:design_c) { create(:design, :with_file, issue: issue) } + + it { is_expected.to contain_exactly(design_a, design_c) } + end + end end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 74ec74e0def..4900743ccb5 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -751,6 +751,14 @@ describe Note do end end + describe '#for_design' do + it 'is true when the noteable is a design' do + note = build(:note, noteable: build(:design)) + + expect(note).to be_for_design + end + end + describe '#to_ability_name' do it 'returns note' do expect(build(:note).to_ability_name).to eq('note') diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 78c0e8aef1a..bcd28538e2c 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -6,6 +6,7 @@ describe Project do include ProjectForksHelper include GitHelpers include ExternalAuthorizationServiceHelpers + using RSpec::Parameterized::TableSyntax it_behaves_like 'having unique enum values' @@ -6058,6 +6059,28 @@ describe Project do end end + describe '#design_management_enabled?' do + let(:project) { build(:project) } + + where(:lfs_enabled, :hashed_storage_enabled, :expectation) do + false | false | false + true | false | false + false | true | false + true | true | true + end + + with_them do + before do + expect(project).to receive(:lfs_enabled?).and_return(lfs_enabled) + allow(project).to receive(:hashed_storage?).with(:repository).and_return(hashed_storage_enabled) + end + + it do + expect(project.design_management_enabled?).to be(expectation) + end + end + end + def finish_job(export_job) export_job.start export_job.finish diff --git a/spec/services/design_management/design_user_notes_count_service_spec.rb b/spec/services/design_management/design_user_notes_count_service_spec.rb new file mode 100644 index 00000000000..f4808542995 --- /dev/null +++ b/spec/services/design_management/design_user_notes_count_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DesignManagement::DesignUserNotesCountService, :use_clean_rails_memory_store_caching do + let_it_be(:design) { create(:design, :with_file) } + + subject { described_class.new(design) } + + it_behaves_like 'a counter caching service' + + describe '#count' do + it 'returns the count of notes' do + create_list(:diff_note_on_design, 3, noteable: design) + + expect(subject.count).to eq(3) + end + end + + describe '#cache_key' do + it 'contains the `VERSION` and `design.id`' do + expect(subject.cache_key).to eq(['designs', 'notes_count', DesignManagement::DesignUserNotesCountService::VERSION, design.id]) + end + end + + # TODO These tests are being temporarily skipped unless run in EE, + # as we are in the process of moving Design Management to FOSS in 13.0 + # in steps. In the current step the services have not yet been moved. + # + # See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283. + describe 'cache invalidation' do + it 'changes when a new note is created' do + skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? + + new_note_attrs = attributes_for(:diff_note_on_design, noteable: design) + + expect do + Notes::CreateService.new(design.project, create(:user), new_note_attrs).execute + end.to change { subject.count }.by(1) + end + + it 'changes when a note is destroyed' do + skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? + + note = create(:diff_note_on_design, noteable: design) + + expect do + Notes::DestroyService.new(note.project, note.author).execute(note) + end.to change { subject.count }.by(-1) + end + end +end diff --git a/spec/services/issues/related_branches_service_spec.rb b/spec/services/issues/related_branches_service_spec.rb index eae35f12560..9f72e499414 100644 --- a/spec/services/issues/related_branches_service_spec.rb +++ b/spec/services/issues/related_branches_service_spec.rb @@ -3,39 +3,103 @@ require 'spec_helper' describe Issues::RelatedBranchesService do - let(:user) { create(:admin) } - let(:issue) { create(:issue) } + let_it_be(:developer) { create(:user) } + let_it_be(:issue) { create(:issue) } + let(:user) { developer } subject { described_class.new(issue.project, user) } + before do + issue.project.add_developer(developer) + end + describe '#execute' do + let(:sha) { 'abcdef' } + let(:repo) { issue.project.repository } + let(:project) { issue.project } + let(:branch_info) { subject.execute(issue) } + + def make_branch + double('Branch', dereferenced_target: double('Target', sha: sha)) + end + before do - allow(issue.project.repository).to receive(:branch_names).and_return(["mpempe", "#{issue.iid}mepmep", issue.to_branch_name, "#{issue.iid}-branch"]) + allow(repo).to receive(:branch_names).and_return(branch_names) end - it "selects the right branches when there are no referenced merge requests" do - expect(subject.execute(issue)).to eq([issue.to_branch_name, "#{issue.iid}-branch"]) + context 'no branches are available' do + let(:branch_names) { [] } + + it 'returns an empty array' do + expect(branch_info).to be_empty + end end - it "selects the right branches when there is a referenced merge request" do - merge_request = create(:merge_request, { description: "Closes ##{issue.iid}", - source_project: issue.project, - source_branch: "#{issue.iid}-branch" }) - merge_request.create_cross_references!(user) + context 'branches are available' do + let(:missing_branch) { "#{issue.to_branch_name}-missing" } + let(:unreadable_branch_name) { "#{issue.to_branch_name}-unreadable" } + let(:pipeline) { build(:ci_pipeline, :success, project: project) } + let(:unreadable_pipeline) { build(:ci_pipeline, :running) } + + let(:branch_names) do + [ + generate(:branch), + "#{issue.iid}doesnt-match", + issue.to_branch_name, + missing_branch, + unreadable_branch_name + ] + end + + before do + { + issue.to_branch_name => pipeline, + unreadable_branch_name => unreadable_pipeline + }.each do |name, pipeline| + allow(repo).to receive(:find_branch).with(name).and_return(make_branch) + allow(project).to receive(:pipeline_for).with(name, sha).and_return(pipeline) + end + + allow(repo).to receive(:find_branch).with(missing_branch).and_return(nil) + end + + it 'selects relevant branches, along with pipeline status where available' do + expect(branch_info).to contain_exactly( + { name: issue.to_branch_name, pipeline_status: an_instance_of(Gitlab::Ci::Status::Success) }, + { name: missing_branch, pipeline_status: be_nil }, + { name: unreadable_branch_name, pipeline_status: be_nil } + ) + end + + context 'the user has access to otherwise unreadable pipelines' do + let(:user) { create(:admin) } + + it 'returns info a developer could not see' do + expect(branch_info.pluck(:pipeline_status)).to include(an_instance_of(Gitlab::Ci::Status::Running)) + end + end + + it 'excludes branches referenced in merge requests' do + merge_request = create(:merge_request, { description: "Closes #{issue.to_reference}", + source_project: issue.project, + source_branch: issue.to_branch_name }) + merge_request.create_cross_references!(user) - referenced_merge_requests = Issues::ReferencedMergeRequestsService - .new(issue.project, user) - .referenced_merge_requests(issue) + referenced_merge_requests = Issues::ReferencedMergeRequestsService + .new(issue.project, user) + .referenced_merge_requests(issue) - expect(referenced_merge_requests).not_to be_empty - expect(subject.execute(issue)).to eq([issue.to_branch_name]) + expect(referenced_merge_requests).not_to be_empty + expect(branch_info.pluck(:name)).not_to include(merge_request.source_branch) + end end - it 'excludes stable branches from the related branches' do - allow(issue.project.repository).to receive(:branch_names) - .and_return(["#{issue.iid}-0-stable"]) + context 'one of the branches is stable' do + let(:branch_names) { ["#{issue.iid}-0-stable"] } - expect(subject.execute(issue)).to eq [] + it 'is excluded' do + expect(branch_info).to be_empty + end end end end diff --git a/spec/support/helpers/design_management_test_helpers.rb b/spec/support/helpers/design_management_test_helpers.rb new file mode 100644 index 00000000000..bf41e2f5079 --- /dev/null +++ b/spec/support/helpers/design_management_test_helpers.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module DesignManagementTestHelpers + def enable_design_management(enabled = true, ref_filter = true) + stub_lfs_setting(enabled: enabled) + stub_feature_flags(design_management_reference_filter_gfm_pipeline: ref_filter) + end + + def delete_designs(*designs) + act_on_designs(designs) { ::DesignManagement::Action.deletion } + end + + def restore_designs(*designs) + act_on_designs(designs) { ::DesignManagement::Action.creation } + end + + def modify_designs(*designs) + act_on_designs(designs) { ::DesignManagement::Action.modification } + end + + def path_for_design(design) + path_options = { vueroute: design.filename } + Gitlab::Routing.url_helpers.designs_project_issue_path(design.project, design.issue, path_options) + end + + def url_for_design(design) + path_options = { vueroute: design.filename } + Gitlab::Routing.url_helpers.designs_project_issue_url(design.project, design.issue, path_options) + end + + def url_for_designs(issue) + Gitlab::Routing.url_helpers.designs_project_issue_url(issue.project, issue) + end + + private + + def act_on_designs(designs, &block) + issue = designs.first.issue + version = build(:design_version, :empty, issue: issue).tap { |v| v.save(validate: false) } + designs.each do |d| + yield.create(design: d, version: version) + end + version + end +end diff --git a/spec/uploaders/design_management/design_v432x230_uploader_spec.rb b/spec/uploaders/design_management/design_v432x230_uploader_spec.rb new file mode 100644 index 00000000000..8c62b6ad6a8 --- /dev/null +++ b/spec/uploaders/design_management/design_v432x230_uploader_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DesignManagement::DesignV432x230Uploader do + include CarrierWave::Test::Matchers + + let(:model) { create(:design_action, :with_image_v432x230) } + let(:upload) { create(:upload, :design_action_image_v432x230_upload, model: model) } + + subject(:uploader) { described_class.new(model, :image_v432x230) } + + it_behaves_like 'builds correct paths', + store_dir: %r[uploads/-/system/design_management/action/image_v432x230/], + upload_path: %r[uploads/-/system/design_management/action/image_v432x230/], + relative_path: %r[uploads/-/system/design_management/action/image_v432x230/], + absolute_path: %r[#{CarrierWave.root}/uploads/-/system/design_management/action/image_v432x230/] + + context 'object_store is REMOTE' do + before do + stub_uploads_object_storage + end + + include_context 'with storage', described_class::Store::REMOTE + + it_behaves_like 'builds correct paths', + store_dir: %r[design_management/action/image_v432x230/], + upload_path: %r[design_management/action/image_v432x230/], + relative_path: %r[design_management/action/image_v432x230/] + end + + describe "#migrate!" do + before do + uploader.store!(fixture_file_upload('spec/fixtures/dk.png')) + stub_uploads_object_storage + end + + it_behaves_like 'migrates', to_store: described_class::Store::REMOTE + it_behaves_like 'migrates', from_store: described_class::Store::REMOTE, to_store: described_class::Store::LOCAL + end + + it 'resizes images', :aggregate_failures do + image_loader = CarrierWave::Test::Matchers::ImageLoader + original_file = fixture_file_upload('spec/fixtures/dk.png') + uploader.store!(original_file) + + expect( + image_loader.load_image(original_file.tempfile.path) + ).to have_attributes( + width: 460, + height: 322 + ) + expect( + image_loader.load_image(uploader.file.file) + ).to have_attributes( + width: 329, + height: 230 + ) + end + + context 'accept whitelist file content type' do + # We need to feed through a valid path, but we force the parsed mime type + # in a stub below so we can set any path. + let_it_be(:path) { File.join('spec', 'fixtures', 'dk.png') } + + where(:mime_type) { described_class::MIME_TYPE_WHITELIST } + + with_them do + include_context 'force content type detection to mime_type' + + it_behaves_like 'accepted carrierwave upload' + end + end + + context 'upload non-whitelisted file content type' do + let_it_be(:path) { File.join('spec', 'fixtures', 'logo_sample.svg') } + + it_behaves_like 'denied carrierwave upload' + end + + context 'upload misnamed non-whitelisted file content type' do + let_it_be(:path) { File.join('spec', 'fixtures', 'not_a_png.png') } + + it_behaves_like 'denied carrierwave upload' + end +end diff --git a/spec/views/projects/issues/_related_branches.html.haml_spec.rb b/spec/views/projects/issues/_related_branches.html.haml_spec.rb index a6817e3fdbf..6c9bbaea38c 100644 --- a/spec/views/projects/issues/_related_branches.html.haml_spec.rb +++ b/spec/views/projects/issues/_related_branches.html.haml_spec.rb @@ -5,23 +5,25 @@ require 'spec_helper' describe 'projects/issues/_related_branches' do include Devise::Test::ControllerHelpers - let(:user) { create(:user) } - let(:project) { create(:project, :repository) } - let(:branch) { project.repository.find_branch('feature') } - let!(:pipeline) { create(:ci_pipeline, project: project, sha: branch.dereferenced_target.id, ref: 'feature') } + let(:pipeline) { build(:ci_pipeline, :success) } + let(:status) { pipeline.detailed_status(build(:user)) } before do - assign(:project, project) - assign(:related_branches, ['feature']) - - project.add_developer(user) - allow(view).to receive(:current_user).and_return(user) + assign(:related_branches, [ + { name: 'other', link: 'link-to-other', pipeline_status: nil }, + { name: 'feature', link: 'link-to-feature', pipeline_status: status } + ]) render end - it 'shows the related branches with their build status' do - expect(rendered).to match('feature') + it 'shows the related branches with their build status', :aggregate_failures do + expect(rendered).to have_text('feature') + expect(rendered).to have_text('other') + expect(rendered).to have_link(href: 'link-to-feature') + expect(rendered).to have_link(href: 'link-to-other') expect(rendered).to have_css('.related-branch-ci-status') + expect(rendered).to have_css('.ci-status-icon') + expect(rendered).to have_css('.related-branch-info') end end |