Merge branch 'delete-designs-v2' into 'master'

Add deletion support for designs

See merge request gitlab-org/gitlab-ee!14656

ee/app/finders/design_management/designs_finder.rb

@@ -15,17 +15,13 @@ module DesignManagement
         return ::DesignManagement::Design.none
       end
 
-      items = issue.designs
-      items = by_visible_at_version(items)
-      items
+      by_visible_at_version(issue.designs)
     end
 
     private
 
     # Returns all designs that existed at a particular design version
     def by_visible_at_version(items)
-      return items unless params[:visible_at_version]
-
       items.visible_at_version(params[:visible_at_version])
     end
   end

ee/app/graphql/ee/types/issue_type.rb

@@ -11,7 +11,10 @@ module EE
               resolve: -> (obj, _args, _ctx) { obj.supports_weight? ? obj.weight : nil }
 
         field :designs, ::Types::DesignManagement::DesignCollectionType,
-              null: true, method: :design_collection
+              null: true, method: :design_collection,
+              deprecation_reason: 'use design_collection'
+
+        field :design_collection, ::Types::DesignManagement::DesignCollectionType, null: true
       end
     end
   end

ee/app/graphql/ee/types/mutation_type.rb

@@ -7,6 +7,7 @@ module EE
 
       prepended do
         mount_mutation ::Mutations::DesignManagement::Upload, calls_gitaly: true
+        mount_mutation ::Mutations::DesignManagement::Delete, calls_gitaly: true
       end
     end
   end

ee/app/graphql/mutations/design_management/base.rb

@@ -13,10 +13,6 @@ module Mutations
                required: true,
                description: "The iid of the issue to modify designs for"
 
-      field :designs, [Types::DesignManagement::DesignType],
-            null: false,
-            description: "The designs that were updated by the mutation"
-
       private
 
       def find_object(project_path:, iid:)

ee/app/graphql/mutations/design_management/delete.rb

+# frozen_string_literal: true
+
+module Mutations
+  module DesignManagement
+    class Delete < Base
+      Errors = ::Gitlab::Graphql::Errors
+
+      graphql_name "DesignManagementDelete"
+
+      argument :filenames, [GraphQL::STRING_TYPE],
+               required: true,
+               description: "The filenames of the designs to delete",
+               prepare: ->(names, _ctx) do
+                 names.presence || (raise Errors::ArgumentError, 'no filenames')
+               end
+
+      field :version, Types::DesignManagement::VersionType,
+            null: true, # null on error
+            description: 'The new version in which the designs are deleted'
+
+      authorize :destroy_design
+
+      def resolve(project_path:, iid:, filenames:)
+        issue = authorized_find!(project_path: project_path, iid: iid)
+        project = issue.project
+        designs = resolve_designs(issue, filenames)
+
+        result = ::DesignManagement::DeleteDesignsService
+          .new(project, current_user, issue: issue, designs: designs)
+          .execute
+
+        {
+          version: result[:version],
+          errors: Array.wrap(result[:message])
+        }
+      end
+
+      private
+
+      # Here we check that:
+      #  * we find exactly as many designs as filenames
+      def resolve_designs(issue, filenames)
+        designs = issue.design_collection.designs_by_filename(filenames)
+
+        validate_all_were_found!(designs, filenames)
+
+        designs
+      end
+
+      def validate_all_were_found!(designs, filenames)
+        found_filenames = designs.map(&:filename)
+        missing = filenames.difference(found_filenames)
+
+        if missing.present?
+          raise Errors::ArgumentError, <<~MSG
+            Not all the designs you named currently exist.
+            The following filenames were not found:
+            #{missing.join(', ')}
+
+            They may have already been deleted.
+          MSG
+        end
+      end
+    end
+  end
+end

ee/app/graphql/mutations/design_management/upload.rb

@@ -11,6 +11,10 @@ module Mutations
 
       authorize :create_design
 
+      field :designs, [Types::DesignManagement::DesignType],
+            null: false,
+            description: "The designs that were uploaded by the mutation"
+
       def resolve(project_path:, iid:, files:)
         issue = authorized_find!(project_path: project_path, iid: iid)
         project = issue.project

ee/app/graphql/types/design_management/design_collection_type.rb

@@ -13,9 +13,8 @@ module Types
             Types::DesignManagement::DesignType.connection_type,
             null: false,
             resolver: Resolvers::DesignManagement::DesignResolver,
-            description: "All visible designs for this collection"
+            description: "All designs for this collection"
       # TODO: allow getting a single design by filename
-      # TODO: when we allow hiding designs, we will also expose a relation
       # exposing all designs
       field :versions,
             Types::DesignManagement::VersionType.connection_type,

ee/app/models/design_management/design.rb

@@ -4,6 +4,7 @@ module DesignManagement
   class Design < ApplicationRecord
     include Noteable
     include Gitlab::FileTypeDetection
+    include Gitlab::Utils::StrongMemoize
 
     belongs_to :project, inverse_of: :designs
     belongs_to :issue
@@ -20,12 +21,50 @@ module DesignManagement
 
     alias_attribute :title, :filename
 
+    # 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
-      created_before_version = DesignManagement::DesignVersion.select(1)
-                               .where("#{table_name}.id = #{DesignManagement::DesignVersion.table_name}.design_id")
-                               .where("#{DesignManagement::DesignVersion.table_name}.version_id <= ?", version)
+      deletion = ::DesignManagement::DesignVersion.events[:deletion]
+      designs = arel_table
+      design_versions = ::DesignManagement::DesignVersion
+        .most_recent.up_to_version(version)
+        .arel.as('most_recent_design_versions')
 
-      where('EXISTS(?)', created_before_version)
+      join = designs.join(design_versions)
+        .on(design_versions[:design_id].eq(designs[:id]))
+
+      joins(join.join_sources).where(design_versions[:event].not_eq(deletion))
+    end
+
+    # 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_design_version&.deletion?
+    end
+
+    def most_recent_design_version
+      strong_memoize(:most_recent_design_version) { design_versions.ordered.last }
     end
 
     def to_reference(_opts)
@@ -37,7 +76,7 @@ module DesignManagement
     end
 
     def new_design?
-      versions.none?
+      strong_memoize(:new_design) { design_versions.none? }
     end
 
     def full_path
@@ -45,9 +84,16 @@ module DesignManagement
     end
 
     def diff_refs
-      return if new_design?
+      strong_memoize(:diff_refs) do
+        head_version.presence && repository.commit(head_version.sha).diff_refs
+      end
+    end
 
-      @diff_refs ||= repository.commit(head_version.sha).diff_refs
+    def clear_version_cache
+      [versions, design_versions].each(&:reset)
+      [:new_design, :diff_refs, :head_sha, :most_recent_design_version].each do |key|
+        clear_memoization(key)
+      end
     end
 
     def repository
@@ -57,7 +103,7 @@ module DesignManagement
     private
 
     def head_version
-      @head_sha ||= versions.ordered.first
+      strong_memoize(:head_sha) { versions.ordered.first }
     end
 
     def validate_file_is_image

ee/app/models/design_management/design_action.rb

+# 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::DesignVersion.events[:creation],
+      update: DesignManagement::DesignVersion.events[:modification],
+      delete: DesignManagement::DesignVersion.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
+    # - gitaly_action [Symbol]: the action that gitlay 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

ee/app/models/design_management/design_collection.rb

@@ -22,5 +22,9 @@ module DesignManagement
     def repository
       project.design_repository
     end
+
+    def designs_by_filename(filenames)
+      designs.current.where(filename: filenames)
+    end
   end
 end

ee/app/models/design_management/design_version.rb

@@ -6,5 +6,29 @@ module DesignManagement
 
     belongs_to :design, class_name: "DesignManagement::Design", inverse_of: :design_versions
     belongs_to :version, class_name: "DesignManagement::Version", inverse_of: :design_versions
+
+    enum event: [:creation, :modification, :deletion]
+
+    # we assume sequential ordering.
+    scope :ordered, -> { order(version_id: :asc) }
+
+    # For each design, only select the most recent design_version
+    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))
+      else
+        raise "Expected a DesignManagement::Version, got #{version}"
+      end
+    end
   end
 end

ee/app/models/design_management/version.rb

@@ -4,6 +4,28 @@ module DesignManagement
   class Version < ApplicationRecord
     include ShaAttribute
 
+    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
     has_many :design_versions
     has_many :designs,
@@ -12,6 +34,7 @@ module DesignManagement
              source: :design,
              inverse_of: :versions
 
+    validates :designs, presence: true
     validates :sha, presence: true
     validates :sha, uniqueness: { case_sensitive: false, scope: :issue_id }
 
@@ -22,19 +45,39 @@ module DesignManagement
     end
     scope :earlier_or_equal_to, -> (version) { where('id <= ?', version) }
     scope :ordered, -> { order(id: :desc) }
+    scope :for_issue, -> (issue) { where(issue: issue) }
 
-    def self.create_for_designs(designs, sha)
-      issue_id = designs.first.issue_id
+    # 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.
+    #
+    # Parameters:
+    # - designs [DesignManagement::DesignAction]:
+    #     the actions that have been performed in the repository.
+    # - sha [String]:
+    #     the SHA of the commit that performed them
+    # returns [DesignManagement::Version]
+    def self.create_for_designs(design_actions, sha)
+      issue_id, not_uniq = design_actions.map(&:issue_id).compact.uniq
+      raise NotSameIssue, 'All designs must belong to the same issue!' if not_uniq
 
-      version = safe_find_or_create_by!(sha: sha, issue_id: issue_id)
+      transaction do
+        version = safe_find_or_create_by(sha: sha, issue_id: issue_id)
+        version.save(validate: false) # We need it to have an ID, validate later
 
-      rows = designs.map do |design|
-        { design_id: design.id, version_id: version.id }
-      end
+        rows = design_actions.map { |action| action.row_attrs(version) }
 
-      Gitlab::Database.bulk_insert(DesignVersion.table_name, rows)
+        Gitlab::Database.bulk_insert(DesignVersion.table_name, rows)
+        version.designs.reset
+        version.validate!
+        design_actions.each(&:performed)
 
-      version
+        version
+      end
+    rescue
+      raise CouldNotCreateVersion.new(sha, issue_id, design_actions)
     end
   end
 end

ee/app/services/design_management/delete_designs_service.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  class DeleteDesignsService < DesignService
+    include RunsDesignActions
+    include OnSuccessCallbacks
+
+    def initialize(project, user, params = {})
+      super
+
+      @designs = params.fetch(:designs)
+    end
+
+    def execute
+      return error('Forbidden!') unless can_delete_designs?
+
+      actions = build_actions
+      version = run_actions(actions)
+
+      success(version: version)
+    end
+
+    def commit_message
+      n = designs.size
+
+      <<~MSG
+      Removed #{n} #{'designs'.pluralize(n)}
+
+      #{formatted_file_list}
+      MSG
+    end
+
+    private
+
+    attr_reader :designs
+
+    def can_delete_designs?
+      Ability.allowed?(current_user, :destroy_design, issue)
+    end
+
+    def build_actions
+      designs.map { |d| design_action(d) }
+    end
+
+    def design_action(design)
+      on_success { counter.count(:delete) }
+
+      DesignManagement::DesignAction.new(design, :delete)
+    end
+
+    def counter
+      ::Gitlab::UsageCounters::DesignsCounter
+    end
+
+    def formatted_file_list
+      designs.map { |design| "- #{design.full_path}" }.join("\n")
+    end
+  end
+end

ee/app/services/design_management/design_service.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  class DesignService < ::BaseService
+    def initialize(project, user, params = {})
+      super
+
+      @issue = params.fetch(:issue)
+    end
+
+    # Accessors common to all subclasses:
+
+    attr_reader :issue
+
+    def target_branch
+      repository.root_ref || "master"
+    end
+
+    def collection
+      issue.design_collection
+    end
+
+    def repository
+      collection.repository
+    end
+
+    def project
+      issue.project
+    end
+  end
+end

ee/app/services/design_management/on_success_callbacks.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  module OnSuccessCallbacks
+    def on_success(&block)
+      success_callbacks.push(block)
+    end
+
+    def success(*_)
+      while cb = success_callbacks.pop
+        cb.call
+      end
+
+      super
+    end
+
+    private
+
+    def success_callbacks
+      @success_callbacks ||= []
+    end
+  end
+end

ee/app/services/design_management/runs_design_actions.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  module RunsDesignActions
+    # this concern requires the following methods to be implemented:
+    #   current_user, target_branch, repository, commit_message
+    def run_actions(actions)
+      repository.create_if_not_exists
+      sha = repository.multi_action(current_user,
+                                    branch_name: target_branch,
+                                    message: commit_message,
+                                    actions: actions.map(&:gitaly_action))
+
+      ::DesignManagement::Version.create_for_designs(actions, sha)
+    end
+  end
+end

ee/app/services/design_management/save_designs_service.rb

 # frozen_string_literal: true
 
 module DesignManagement
-  class SaveDesignsService < ::BaseService
+  class SaveDesignsService < DesignService
+    include RunsDesignActions
+    include OnSuccessCallbacks
+
     MAX_FILES = 10
 
     def initialize(project, user, params = {})
       super
 
-      @issue = params.fetch(:issue)
       @files = params.fetch(:files)
-      @success_callbacks = []
     end
 
     def execute
       return error("Not allowed!") unless can_create_designs?
       return error("Only #{MAX_FILES} files are allowed simultaneously") if files.size > MAX_FILES
 
-      save_designs!
+      actions = build_actions
+      run_actions(actions)
 
-      success({ designs: updated_designs })
-    rescue ::Gitlab::Git::BaseError, ::ActiveRecord::RecordInvalid => e
+      success({ designs: actions.map(&:design) })
+    rescue ::ActiveRecord::RecordInvalid => e
       error(e.message)
     end
 
     private
 
-    attr_reader :files, :issue
-
-    def success(*_)
-      while cb = @success_callbacks.pop
-        cb.call
-      end
-
-      super
-    end
-
-    def on_success(&block)
-      @success_callbacks.push(block)
-    end
-
-    def save_designs!
-      commit_sha = create_and_commit_designs!
-      ::DesignManagement::Version.create_for_designs(updated_designs, commit_sha)
-    end
+    attr_reader :files
+    attr_accessor :paths_in_repo
 
-    def create_and_commit_designs!
+    def build_actions
       repository.create_if_not_exists
 
-      # Do not inline `build_repository_action` here!
-      # We have to do this as two *separate* calls to #map so that the call
-      # to `new_file?` does not accidentally cache the wrong data half-way
-      # through the operation.
-      corresponding_designs = files.map do |file|
+      designs = files.map do |file|
         collection.find_or_create_design!(filename: file.original_filename)
       end
 
-      actions = files.zip(corresponding_designs).map do |(file, design)|
-        build_repository_action(file, design)
-      end
+      # Needs to be called before any call to build_design_action
+      cache_existence(designs)
 
-      repository.multi_action(current_user,
-                              branch_name: target_branch,
-                              message: commit_message,
-                              actions: actions)
+      files.zip(designs).map do |(file, design)|
+        build_design_action(file, design)
+      end
     end
 
-    def build_repository_action(file, design)
+    def build_design_action(file, design)
       action = new_file?(design) ? :create : :update
+      content = file_content(file, design.full_path)
       on_success { ::Gitlab::UsageCounters::DesignsCounter.count(action) }
 
-      {
-        action: action,
-        file_path: design.full_path,
-        content: file_content(file, design.full_path)
-      }
-    end
-
-    def collection
-      issue.design_collection
-    end
-
-    def repository
-      project.design_repository
-    end
-
-    def project
-      issue.project
-    end
-
-    def target_branch
-      repository.root_ref || "master"
+      DesignManagement::DesignAction.new(design, action, content)
     end
 
     def commit_message
@@ -108,16 +69,16 @@ module DesignManagement
       @filenames ||= files.map(&:original_filename)
     end
 
-    def updated_designs
-      @updated_designs ||= collection.designs.select { |design| filenames.include?(design.filename) }
-    end
-
     def can_create_designs?
       Ability.allowed?(current_user, :create_design, issue)
     end
 
     def new_file?(design)
-      design.new_design? && existing_metadata.none? { |blob| blob.path == design.full_path }
+      design.new_design? && !on_disk?(design)
+    end
+
+    def on_disk?(design)
+      paths_in_repo === design.full_path
     end
 
     def file_content(file, full_path)
@@ -125,11 +86,9 @@ module DesignManagement
       transformer.new_file(full_path, file.to_io).content
     end
 
-    def existing_metadata
-      @existing_metadata ||= begin
-                               paths = updated_designs.map(&:full_path)
-                               repository.blobs_metadata(paths)
-                             end
+    def cache_existence(designs)
+      paths = designs.map(&:full_path)
+      self.paths_in_repo = repository.blobs_metadata(paths).map(&:path).to_set
     end
   end
 end

ee/changelogs/unreleased/delete-designs-v2.yml

+---
+title: Add deletion support for designs
+merge_request: 14656
+type: added

ee/db/fixtures/development/27_designs.rb

+# frozen_string_literal: true
+
+class SeedDesigns
+  SETTINGS = %i[
+    n_issues
+    max_designs_per_issue
+    max_versions_per_issue
+    max_designs_per_version
+  ].freeze
+
+  attr_reader(*SETTINGS)
+
+  def initialize(settings)
+    SETTINGS.each do |k|
+      instance_variable_set("@#{k}".to_sym, settings.fetch(k))
+    end
+  end
+
+  def uploads
+    @uploads ||= ["dk.png", "rails_sample.jpg"]
+      .map { |fn| upload(fn) }
+      .cycle(max_designs_per_issue)
+      .map { |upload| upload.rename(random_file_name) }
+  end
+
+  def random_file_name
+    "#{FFaker::Product.product_name}-#{FFaker::Product.unique.model}"
+  end
+
+  Upload = Struct.new(:original_filename, :to_io) do
+    def rename(name)
+      Upload.new("#{name}.#{File.extname(original_filename)}", to_io)
+    end
+  end
+
+  def upload(filename)
+    content = File.open("spec/fixtures/#{filename}", 'r') do |f|
+      StringIO.new(f.read)
+    end
+
+    Upload.new(filename, content)
+  end
+
+  def as_action(design)
+    next_action = case design.status
+                  when :deleted, :new
+                    :create
+                  when :current
+                    [:update, :delete].sample
+                  end
+
+    DesignManagement::DesignAction.new(
+      design,
+      next_action,
+      next_action == :delete ? nil : uploads.sample.to_io
+    )
+  end
+
+  def create_version(repo, devs, to_change, version_number)
+    user = devs.sample
+    actions = to_change.map { |design| as_action(design) }
+    sha = repo.multi_action(user, branch_name: 'master',
+                                 message: "version #{version_number}",
+                                 actions: actions.map(&:gitaly_action))
+    version = DesignManagement::Version.create_for_designs(actions, sha)
+    if version.valid?
+      print('.' * to_change.size)
+    else
+      print('F' * to_change.size)
+      version.errors.each { |e| warn(e) }
+    end
+  end
+
+  def create_designs(project, issue, repo, devs)
+    files = uploads.sample(Random.rand(2..max_designs_per_issue))
+
+    files.in_groups_of(10).map(&:compact).select(&:present?).flat_map do |fs|
+      user = devs.sample
+      service = DesignManagement::SaveDesignsService.new(project, user,
+                                                         issue: issue,
+                                                         files: fs)
+
+      message, designs = service.execute.values_at(:message, :designs)
+      if message
+        print('F' * fs.size)
+        warn(message)
+      else
+        print('.' * designs.size)
+      end
+
+      designs || []
+    end
+  end
+
+  def run
+    Issue.all.sample(n_issues).each do |issue|
+      project = issue.project
+      repo = project.design_repository
+      devs = project.team.developers.all
+
+      repo.create_if_not_exists
+
+      # All designs get created at least once
+      designs = create_designs(project, issue, repo, devs)
+
+      Random.rand(max_versions_per_issue).times do |i|
+        to_change = designs.sample(Random.rand(1..max_designs_per_version))
+        create_version(repo, devs, to_change, i)
+      end
+    end
+  end
+
+  def warn(msg)
+    Rails.logger.warn(msg) # rubocop: disable Gitlab/RailsLogger
+  end
+end
+
+Gitlab::Seeder.quiet do
+  clear                   = ENV.fetch('DESIGN_MANAGEMENT_SEED_CLEAR', false)
+  n_issues                = ENV.fetch('DESIGN_MANAGEMENT_SEED_N_ISSUES', 3).to_i
+  max_designs_per_issue   = ENV.fetch('DESIGN_MANAGEMENT_SEED_DESIGNS_PER_ISSUE', 5).to_i
+  max_versions_per_issue  = ENV.fetch('DESIGN_MANAGEMENT_SEED_VERSIONS_PER_ISSUE', 5).to_i
+  max_designs_per_version = ENV.fetch('DESIGN_MANAGEMENT_SEED_DESIGNS_PER_VERSION', 5).to_i
+  max_designs_per_issue   = [2, max_designs_per_issue].max
+
+  flags = %i[design_management design_management_flag].map do |flag|
+    old = Feature.enabled?(flag)
+    Feature.enable(flag)
+    [flag, old]
+  end.to_h
+
+  DesignManagement::Design.delete_all if clear
+
+  seed = SeedDesigns.new(n_issues: n_issues,
+                         max_designs_per_issue: max_designs_per_issue,
+                         max_versions_per_issue: max_versions_per_issue,
+                         max_designs_per_version: max_designs_per_version)
+
+  seed.run
+ensure
+  flags.each do |(flag, old_value)|
+    old_value ? Feature.enable(flag) : Feature.disable(flag)
+  end
+end

ee/spec/factories/design_management/designs.rb

@@ -2,10 +2,38 @@
 
 FactoryBot.define do
   factory :design, class: DesignManagement::Design do
-    issue
+    issue { create(:issue) }
     project { issue.project }
     sequence(:filename) { |n| "homescreen-#{n}.jpg" }
 
+    create_versions = ->(design, evaluator, commit_version) do
+      unless evaluator.versions_count.zero?
+        project = design.project
+        repository = project.design_repository
+        repository.create_if_not_exists
+        dv_table_name = DesignManagement::DesignVersion.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: design.issue)
+          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
+    end
+
     trait :with_lfs_file do
       with_file
 
@@ -14,35 +42,50 @@ FactoryBot.define do
       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 if you want your designs to be as true-to-life as possible,
+    # with correctly made 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|
-        unless evaluator.versions_count.zero?
-          project = design.project
-          repository = project.design_repository
-          repository.create_if_not_exists
-
-          evaluator.versions_count.times do |i|
-            actions = [{
-              action: i.zero? ? :create : :update, # First version is :create, successive versions are :update
-              file_path: design.full_path,
-              content: evaluator.file
-            }]
-
-            sha = repository.multi_action(
-              project.creator,
-              branch_name: 'master',
-              message: "Automatically created file #{design.filename}",
-              actions: actions
-            )
-
-            FactoryBot.create(:design_version, designs: [design], sha: sha)
-          end
+        project = design.project
+        repository = project.design_repository
+
+        commit_version = ->(action) do
+          repository.multi_action(
+            project.creator,
+            branch_name: 'master',
+            message: "#{action.action} for #{design.filename}",
+            actions: [action.gitaly_action]
+          )
         end
+
+        create_versions[design, evaluator, commit_version]
       end
     end
   end

ee/spec/factories/design_management/versions.rb

@@ -4,5 +4,18 @@ 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) }
+
+    # Warning: this will intentionally result in an invalid version!
+    trait :empty do
+      transient do
+        no_designs true
+      end
+    end
+
+    after(:build) do |version, evaluator|
+      unless evaluator.try(:no_designs) || version.designs.present?
+        version.designs << create(:design, issue: version.issue)
+      end
+    end
   end
 end

ee/spec/finders/design_management/designs_finder_spec.rb

@@ -47,13 +47,13 @@ describe DesignManagement::DesignsFinder do
           let(:second_version) { all_versions.first }
 
           context 'when argument is the first version' do
-            let(:params) { { visible_at_version: first_version.id } }
+            let(:params) { { visible_at_version: first_version } }
 
             it { is_expected.to eq([design1]) }
           end
 
           context 'when argument is the second version' do
-            let(:params) { { visible_at_version: second_version.id } }
+            let(:params) { { visible_at_version: second_version } }
 
             it { is_expected.to contain_exactly(design2, design1) }
           end

ee/spec/graphql/mutations/design_management/delete_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Mutations::DesignManagement::Delete do
+  include DesignManagementTestHelpers
+
+  Errors = Gitlab::Graphql::Errors
+
+  let(:issue) { create(:issue) }
+  let(:current_designs) { issue.designs.current }
+  let(:user) { issue.author }
+  let(:project) { issue.project }
+  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) }
+  let(:filenames) { [design_a, design_b, design_c].map(&:filename) }
+
+  let(:mutation) { described_class.new(object: nil, context: { current_user: user }) }
+
+  def run_mutation
+    mutation = described_class.new(object: nil, context: { current_user: user })
+    mutation.resolve(project_path: project.full_path, iid: issue.iid, filenames: filenames)
+  end
+
+  describe '#resolve' do
+    let(:expected_response) do
+      { errors: [], version: DesignManagement::Version.for_issue(issue).ordered.first }
+    end
+
+    shared_examples "failures" do |error: Errors::ResourceNotAvailable|
+      it "raises #{error.name}" do
+        expect { run_mutation }.to raise_error(error)
+      end
+    end
+
+    shared_examples "resource not available" do
+      it_behaves_like "failures"
+    end
+
+    context "when the feature is not available" do
+      before do
+        enable_design_management(false)
+      end
+
+      it_behaves_like "resource not available"
+    end
+
+    context "when the feature is available" do
+      before do
+        enable_design_management(true)
+      end
+
+      context "when the user is not allowed to delete designs" do
+        let(:user) { create(:user) }
+
+        it_behaves_like "resource not available"
+      end
+
+      context 'deleting an already deleted file' do
+        before do
+          run_mutation
+        end
+
+        it 'fails with an argument error' do
+          expect { run_mutation }.to raise_error(Errors::ArgumentError)
+        end
+      end
+
+      context "when deleting all the designs" do
+        let(:response) { run_mutation }
+
+        it "returns a new version, and no errors" do
+          expect(response).to include(expected_response)
+        end
+
+        describe 'the current designs' do
+          before do
+            run_mutation
+          end
+
+          it 'is empty' do
+            expect(current_designs).to be_empty
+          end
+        end
+
+        it 'runs no more than 27 queries' do
+          filenames.each(&:present?) # ignore setup
+          # Queries: as of 2019-08-08
+          # -------------
+          # 01. routing query
+          # 02. find project by id
+          # 03. project.project_features
+          # 04. find namespace by id and type
+          # 05,06. project.authorizations for user (same query twice)
+          # 07. find issue by iid
+          # 08. find project by id
+          # 09. find namespace by id
+          # 10. find group namespace by id
+          # 11. project.authorizations for user (same query as 5)
+          # 12. project.project_features (same query as 3)
+          # 13. project.authorizations for user (same query as 5)
+          # 14. current designs by filename and issue
+          # 15, 16 project.authorizations for user (same query as 5)
+          # 17. find route by id and source_type
+          # ------------- our queries are below:
+          # 18. start transaction 1
+          # 19.   start transaction 2
+          # 20.     find version by sha and issue
+          # 21.     exists version with sha and issue?
+          # 22.   leave transaction 2
+          # 23.   create version with sha and issue
+          # 24.   create design-version links
+          # 25.   validate version.design_versions.present?
+          # 26.   validate version.sha is unique
+          # 27. leave transaction 1
+          #
+          expect { run_mutation }.not_to exceed_query_limit(27)
+        end
+      end
+
+      context "when deleting a design" do
+        let(:filenames) { [design_a.filename] }
+        let(:response) { run_mutation }
+
+        it "returns the expected response" do
+          expect(response).to include(expected_response)
+        end
+
+        describe 'the current designs' do
+          before do
+            run_mutation
+          end
+
+          it 'does contain designs b and c' do
+            expect(current_designs).to contain_exactly(design_b, design_c)
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/graphql/types/design_management/design_type_spec.rb

@@ -8,7 +8,18 @@ describe GitlabSchema.types['Design'] do
   it { expect(described_class.interfaces).to include(Types::Notes::NoteableType.to_graphql) }
 
   it 'exposes the expected fields' do
-    expected_fields = [:id, :project, :issue, :filename, :image, :versions, :discussions, :notes, :diff_refs, :full_path]
+    expected_fields = %i[
+      id
+      project
+      issue
+      filename
+      image
+      versions
+      discussions
+      notes
+      diff_refs
+      full_path
+    ]
 
     is_expected.to have_graphql_fields(*expected_fields)
   end

ee/spec/models/design_management/design_action_spec.rb

+# 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

ee/spec/models/design_management/design_collection_spec.rb

@@ -2,7 +2,10 @@
 require 'spec_helper'
 
 describe DesignManagement::DesignCollection do
-  let(:issue) { create(:issue) }
+  include DesignManagementTestHelpers
+
+  set(:issue) { create(:issue) }
+
   subject(:collection) { described_class.new(issue) }
 
   describe ".find_or_create_design!" do
@@ -51,4 +54,29 @@ describe DesignManagement::DesignCollection 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

ee/spec/models/design_management/design_spec.rb

@@ -3,6 +3,14 @@
 require 'rails_helper'
 
 describe DesignManagement::Design do
+  include DesignManagementTestHelpers
+
+  set(:issue) { create(:issue) }
+  set(:design1) { create(:design, :with_versions, issue: issue, versions_count: 1) }
+  set(:design2) { create(:design, :with_versions, issue: issue, versions_count: 1) }
+  set(:design3) { create(:design, :with_versions, issue: issue, versions_count: 1) }
+  set(: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) }
@@ -25,48 +33,195 @@ describe DesignManagement::Design do
 
       expect(design).not_to be_valid
       expect(design.errors[:filename].first)
-        .to match /Only these extensions are supported/
+        .to match %r/Only these extensions are supported/
     end
   end
 
   describe 'scopes' do
     describe '.visible_at_version' do
-      let!(:design1) { create(:design, :with_file, versions_count: 1) }
-      let!(:design2) { create(:design, :with_file, versions_count: 1) }
-      let(:first_version) { DesignManagement::Version.ordered.last }
-      let(:second_version) { DesignManagement::Version.ordered.first }
-
-      it 'returns just designs that existed at that version' do
-        expect(described_class.visible_at_version(first_version)).to eq([design1])
-        expect(described_class.visible_at_version(second_version)).to contain_exactly(design1, design2)
+      let(:versions) { DesignManagement::Version.where(issue: issue).ordered }
+
+      context 'at oldest version' do
+        let(:version) { versions.last }
+
+        it 'finds the first design only' do
+          expect(described_class.visible_at_version(version)).to contain_exactly(design1)
+        end
+      end
+
+      context 'at version 2' do
+        let(:version) { versions.second }
+
+        it 'finds the first and second designs' do
+          expect(described_class.visible_at_version(version)).to contain_exactly(design1, design2)
+        end
+      end
+
+      context 'at latest version' do
+        let(:version) { versions.first }
+
+        it 'finds designs' do
+          expect(described_class.visible_at_version(version)).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(described_class.visible_at_version(version)).to contain_exactly(design1, design2, design3)
+        end
       end
 
-      it 'can be passed either a DesignManagement::Version or an ID' do
-        [first_version, first_version.id].each do |arg|
-          expect(described_class.visible_at_version(arg)).to eq([design1])
+      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 '.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 '#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
-    set(:versions) { create(:design_version) }
-    set(:design) { create(:design, versions: [versions]) }
+    let(:design) { design1 }
 
     it "is false when there are versions" do
-      expect(design.new_design?).to be_falsy
+      expect(design1).not_to be_new_design
     end
 
     it "is true when there are no versions" do
-      expect(build(:design).new_design?).to be_truthy
+      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 versions are loaded" do
-      design.versions.map(&:id)
+      design.design_versions.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
 
@@ -84,18 +239,32 @@ describe DesignManagement::Design do
   end
 
   describe '#diff_refs' do
-    it "builds diff refs based on the first commit and it's for the design" do
-      design = create(:design, :with_file, versions_count: 3)
+    let(:design) { create(:design, :with_file, versions_count: versions_count) }
+
+    context 'there are several versions' do
+      let(:versions_count) { 3 }
 
-      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)
+      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
 
-    it 'builds diff refs based on the empty tree if there was only one version' do
-      design = create(:design, :with_file, versions_count: 1)
+    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.base_sha).to eq(Gitlab::Git::BLANK_SHA)
-      expect(design.diff_refs.head_sha).to eq(design.diff_refs.head_sha)
+      expect(design.diff_refs).to be_nil
     end
   end
 

ee/spec/models/design_management/design_version_spec.rb

@@ -6,4 +6,72 @@ describe DesignManagement::DesignVersion do
     it { is_expected.to belong_to(:design) }
     it { is_expected.to belong_to(:version) }
   end
+
+  describe 'scopes' do
+    describe '.most_recent' do
+      set(:design_a) { create(:design) }
+      set(:design_b) { create(:design) }
+      set(: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
+      set(:issue) { create(:issue) }
+      set(:design_a) { create(:design, issue: issue) }
+      set(: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.
+      set(:oldest) { create(:design_version, designs: [design_a, design_b]) }
+      set(:middle) { create(:design_version, designs: [design_a, design_b]) }
+      set(: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 '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
+  end
 end

ee/spec/models/design_management/version_spec.rb

@@ -8,9 +8,7 @@ describe DesignManagement::Version do
 
     it 'constrains the designs relation correctly' do
       design = create(:design)
-      version = create(:design_version)
-
-      version.designs << design
+      version = create(:design_version, designs: [design])
 
       expect { version.designs << design }.to raise_error(ActiveRecord::RecordNotUnique)
     end
@@ -29,6 +27,7 @@ describe DesignManagement::Version do
 
     it { is_expected.to be_valid }
     it { is_expected.to validate_presence_of(:sha) }
+    it { is_expected.to validate_presence_of(:designs) }
     it { is_expected.to validate_uniqueness_of(:sha).scoped_to(:issue_id).case_insensitive }
   end
 
@@ -61,13 +60,147 @@ describe DesignManagement::Version do
     end
   end
 
-  describe ".bulk_create" do
+  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
+
+    set(:issue) { create(:issue) }
+    set(:design_a) { create(:design, issue: issue) }
+    set(:design_b) { create(:design, issue: issue) }
+    let(:designs) { [design_a, design_b] }
+
+    describe 'the error raised when there are no actions' do
+      let(:sha) { 'f00' }
+
+      def call_with_empty_actions
+        described_class.create_for_designs([], sha)
+      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 'then error raised when the designs come from different issues' do
+      let(:sha) { 'f00' }
+      let(:designs) { create_list(:design, 2) }
+      let(:actions) { as_actions(designs) }
+
+      def call_with_mismatched_designs
+        described_class.create_for_designs(actions, sha)
+      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') 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, '') rescue nil
+      end.not_to change { DesignManagement::DesignVersion.count }
+    end
+
     it "creates a version and links it to multiple designs" do
-      designs = create_list(:design, 2)
+      actions = as_actions(designs, :create)
 
-      version = described_class.create_for_designs(designs, "abc")
+      version = described_class.create_for_designs(actions, "abc")
 
       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")
+
+      expect(designs.map(&:most_recent_design_version)).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")
+
+      expect(version.issue).to eq(issue)
+    end
+
+    it 'modifies designs if git updated them' do
+      actions = as_actions(designs, :update)
+
+      described_class.create_for_designs(actions, "abc")
+
+      expect(designs.map(&:most_recent_design_version)).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")
+
+      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")
+      described_class.create_for_designs(as_actions(designs, :delete), "def")
+
+      expect(designs).to all(be_deleted)
+
+      described_class.create_for_designs(as_actions(designs, :create), "ghi")
+
+      expect(designs.map(&:most_recent_design_version)).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")
+
+      expect do
+        described_class.create_for_designs(actions, "after")
+      end.to change { current_version_id(design_a) }
     end
   end
 end

ee/spec/models/issue_spec.rb

@@ -365,4 +365,37 @@ describe Issue do
       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

ee/spec/requests/api/graphql/mutations/design_management/delete_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe "deleting designs" do
+  include GraphqlHelpers
+  include DesignManagementTestHelpers
+
+  let(:developer) { create(:user) }
+  let(:current_user) { developer }
+  let(:issue) { create(:issue) }
+  let(:project) { issue.project }
+  let(:designs) { create_designs }
+  let(:variables) { {} }
+
+  let(:mutation) do
+    input = {
+      project_path: project.full_path,
+      iid: issue.iid,
+      filenames: designs.map(&:filename)
+    }.merge(variables)
+    graphql_mutation(:design_management_delete, input)
+  end
+
+  let(:mutation_response) { graphql_mutation_response(:design_management_delete) }
+
+  def mutate!
+    post_graphql_mutation(mutation, current_user: current_user)
+  end
+
+  before do
+    enable_design_management
+
+    project.add_developer(developer)
+  end
+
+  shared_examples 'a failed request' do
+    let(:the_error) { be_present }
+
+    it 'reports an error' do
+      mutate!
+
+      expect(graphql_errors).to include(a_hash_including('message' => the_error))
+    end
+  end
+
+  context 'the designs list is empty' do
+    it_behaves_like 'a failed request' do
+      let(:designs) { [] }
+      let(:the_error) { eq 'no filenames' }
+    end
+  end
+
+  context 'the designs list contains filenames we cannot find' do
+    it_behaves_like 'a failed request' do
+      let(:designs) { %w/foo bar baz/.map { |fn| OpenStruct.new(filename: fn) } }
+      let(:the_error) { a_string_matching %r/filenames were not found/ }
+    end
+  end
+
+  context 'the current user does not have developer access' do
+    it_behaves_like 'a failed request' do
+      let(:current_user) { create(:user) }
+      let(:the_error) { a_string_matching %r/you don't have permission/ }
+    end
+  end
+
+  context "when the issue does not exist" do
+    it_behaves_like 'a failed request' do
+      let(:variables) { { iid: "1234567890" } }
+      let(:the_error) { a_string_matching %r/does not exist/ }
+    end
+  end
+
+  context "when saving the designs raises an error" do
+    let(:designs) { create_designs(1) }
+
+    it "responds with errors" do
+      expect_next_instance_of(::DesignManagement::DeleteDesignsService) do |service|
+        expect(service)
+          .to receive(:execute)
+          .and_return({ status: :error, message: "Something went wrong" })
+      end
+
+      mutate!
+
+      expect(mutation_response).to include('errors' => include(eq "Something went wrong"))
+    end
+  end
+
+  context 'one of the designs is already deleted' do
+    let(:designs) do
+      create_designs(2).push(create(:design, :with_file, deleted: true, issue: issue))
+    end
+
+    it 'reports an error' do
+      mutate!
+
+      expect(graphql_errors).to be_present
+    end
+  end
+
+  context 'when the user names designs to delete' do
+    before do
+      create_designs(1)
+    end
+
+    let!(:designs) { create_designs(2) }
+
+    it 'deletes the designs' do
+      expect { mutate! }
+        .to change { issue.reset.designs.current.count }.from(3).to(1)
+    end
+
+    it 'has no errors' do
+      mutate!
+
+      expect(mutation_response).to include('errors' => be_empty)
+    end
+  end
+
+  private
+
+  def create_designs(how_many = 2)
+    create_list(:design, how_many, :with_file, issue: issue)
+  end
+end

ee/spec/requests/api/graphql/mutations/design_management/upload_spec.rb

@@ -31,16 +31,23 @@ describe "uploading designs" do
   it "returns an error if the user is not allowed to upload designs" do
     post_graphql_mutation(mutation, current_user: create(:user))
 
-    expect(graphql_errors).not_to be_empty
+    expect(graphql_errors).to be_present
   end
 
-  it "responds with the created designs" do
+  it 'succeeds' do
     post_graphql_mutation(mutation, current_user: current_user)
 
-    designs = mutation_response["designs"]
+    expect(graphql_errors).not_to be_present
+  end
+
+  it "responds with the created designs" do
+    post_graphql_mutation(mutation, current_user: current_user)
 
-    expect(designs.size).to eq(1)
-    expect(designs.first["filename"]).to eq("dk.png")
+    expect(mutation_response).to include(
+      'designs' => a_collection_containing_exactly(
+        a_hash_including('filename' => 'dk.png')
+      )
+    )
   end
 
   context "when the issue does not exist" do

ee/spec/services/design_management/delete_designs_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe DesignManagement::DeleteDesignsService do
+  include DesignManagementTestHelpers
+  set(:project) { create(:project) }
+  set(:issue) { create(:issue, project: project) }
+  set(:user) { create(:user) }
+  let(:designs) { create_designs }
+
+  subject(:service) { described_class.new(project, user, issue: issue, designs: designs) }
+
+  # Defined as a method so that the reponse is not cached. We also construct
+  # a new service executor each time to avoid the intermediate cached values
+  # it constructs during its execution.
+  def run_service(delenda = nil)
+    service = described_class.new(project, user, issue: issue, designs: delenda || designs)
+    service.execute
+  end
+
+  let(:response) { run_service }
+
+  shared_examples 'a service error' do
+    it 'returns an error', :aggregate_failures do
+      expect(response).to include(status: :error)
+    end
+  end
+
+  shared_examples 'a top-level error' do
+    let(:expected_error) { StandardError }
+    it 'raises an en expected error', :aggregate_failures do
+      expect { run_service }.to raise_error(expected_error)
+    end
+  end
+
+  shared_examples 'a success' do
+    it 'returns successfully', :aggregate_failures do
+      expect(response).to include(status: :success)
+    end
+  end
+
+  before do
+    enable_design_management(enabled)
+    project.add_developer(user)
+  end
+
+  describe "#execute" do
+    context "when the feature is not available" do
+      let(:enabled) { false }
+
+      it_behaves_like "a service error"
+    end
+
+    context "when the feature is available" do
+      let(:enabled) { true }
+
+      it 'is able to delete designs' do
+        expect(service.send(:can_delete_designs?)).to be true
+      end
+
+      context 'no designs were passed' do
+        let(:designs) { [] }
+
+        it_behaves_like "a top-level error"
+
+        it 'does not log any events' do
+          counter = ::Gitlab::UsageCounters::DesignsCounter
+          expect { run_service rescue nil }.not_to change { counter.totals }
+        end
+      end
+
+      context 'one design is passed' do
+        before do
+          create_designs(2)
+        end
+
+        let!(:designs) { create_designs(1) }
+
+        it 'removes that design' do
+          expect { run_service }.to change { issue.designs.current.count }.from(3).to(2)
+        end
+
+        it 'logs a deletion event' do
+          counter = ::Gitlab::UsageCounters::DesignsCounter
+          expect { run_service }.to change { counter.read(:delete) }.by(1)
+        end
+
+        it 'creates a new verison' do
+          expect { run_service }.to change { DesignManagement::Version.where(issue: issue).count }.by(1)
+        end
+
+        it 'returns the new version' do
+          version = response[:version]
+
+          expect(version).to eq(DesignManagement::Version.for_issue(issue).ordered.first)
+        end
+
+        it_behaves_like "a success"
+
+        it 'removes the design from the current design list' do
+          run_service
+
+          expect(issue.designs.current).not_to include(designs.first)
+        end
+
+        it 'marks the design as deleted' do
+          expect { run_service }
+            .to change { designs.first.deleted? }.from(false).to(true)
+        end
+      end
+
+      context 'more than one design is passed' do
+        before do
+          create_designs(1)
+        end
+
+        let!(:designs) { create_designs(2) }
+
+        it 'removes those designs' do
+          expect { run_service }
+            .to change { issue.designs.current.count }.from(3).to(1)
+        end
+
+        it 'logs the correct number of deletion events' do
+          counter = ::Gitlab::UsageCounters::DesignsCounter
+          expect { run_service }.to change { counter.read(:delete) }.by(2)
+        end
+
+        it_behaves_like "a success"
+
+        context 'after executing the service' do
+          let(:deleted_designs) { designs.map(&:reset) }
+
+          let!(:version) { run_service[:version] }
+
+          it 'removes the removed designs from the current design list' do
+            expect(issue.designs.current).not_to include(*deleted_designs)
+          end
+
+          it 'does not make the designs impossible to find' do
+            expect(issue.designs).to include(*deleted_designs)
+          end
+
+          it 'associates the new version with all the designs' do
+            current_versions = deleted_designs.map { |d| d.most_recent_design_version.version }
+            expect(current_versions).to all(eq version)
+          end
+
+          it 'marks all deleted designs as deleted' do
+            expect(deleted_designs).to all(be_deleted)
+          end
+
+          it 'marks all deleted designs with the same deletion version' do
+            expect(deleted_designs.map { |d| d.most_recent_design_version.version_id }.uniq)
+              .to have_attributes(size: 1)
+          end
+        end
+      end
+
+      describe 'scalability' do
+        before do
+          run_service(create_designs(1)) # ensure project, issue, etc are created
+        end
+
+        it 'makes the same number of DB requests for one design as for several' do
+          one = create_designs(1)
+          many = create_designs(5)
+
+          baseline = ActiveRecord::QueryRecorder.new { run_service(one) }
+
+          expect { run_service(many) }.not_to exceed_query_limit(baseline)
+        end
+      end
+    end
+  end
+
+  private
+
+  def create_designs(how_many = 2)
+    create_list(:design, how_many, :with_lfs_file, issue: issue)
+  end
+end

ee/spec/support/helpers/design_management_test_helpers.rb

 # frozen_string_literal: true
 
 module DesignManagementTestHelpers
-  def enable_design_management
-    stub_licensed_features(design_management: true)
-    stub_lfs_setting(enabled: true)
+  def enable_design_management(enabled = true)
+    stub_licensed_features(design_management: enabled)
+    stub_lfs_setting(enabled: enabled)
+  end
+
+  def delete_designs(*designs)
+    act_on_designs(designs) { ::DesignManagement::DesignVersion.deletion }
+  end
+
+  def restore_designs(*designs)
+    act_on_designs(designs) { ::DesignManagement::DesignVersion.creation }
+  end
+
+  def modify_designs(*designs)
+    act_on_designs(designs) { ::DesignManagement::DesignVersion.modification }
+  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