Merge branch '218003-iteration_changes_notifications' into 'master'

Create SystemNote when Issue Iteration is changed

See merge request gitlab-org/gitlab!32346

app/helpers/milestones_routing_helper.rb → app/helpers/timeboxes_routing_helper.rb

 # frozen_string_literal: true
 
-module MilestonesRoutingHelper
+module TimeboxesRoutingHelper
   def milestone_path(milestone, *args)
     if milestone.group_milestone?
       group_milestone_path(milestone.group, milestone, *args)
@@ -17,3 +17,5 @@ module MilestonesRoutingHelper
     end
   end
 end
+
+TimeboxesRoutingHelper.prepend_if_ee('EE::TimeboxesRoutingHelper')

app/models/concerns/timebox.rb

@@ -7,6 +7,7 @@ module Timebox
   include CacheMarkdownField
   include Gitlab::SQL::Pattern
   include IidRoutes
+  include Referable
   include StripAttribute
 
   TimeboxStruct = Struct.new(:title, :name, :id) do
@@ -122,6 +123,35 @@ module Timebox
     end
   end
 
+  ##
+  # Returns the String necessary to reference a Timebox in Markdown. Group
+  # timeboxes only support name references, and do not support cross-project
+  # references.
+  #
+  # format - Symbol format to use (default: :iid, optional: :name)
+  #
+  # Examples:
+  #
+  #   Milestone.first.to_reference                           # => "%1"
+  #   Iteration.first.to_reference(format: :name)            # => "*iteration:\"goal\""
+  #   Milestone.first.to_reference(cross_namespace_project)  # => "gitlab-org/gitlab-foss%1"
+  #   Iteration.first.to_reference(same_namespace_project)   # => "gitlab-foss*iteration:1"
+  #
+  def to_reference(from = nil, format: :name, full: false)
+    format_reference = timebox_format_reference(format)
+    reference = "#{self.class.reference_prefix}#{format_reference}"
+
+    if project
+      "#{project.to_reference_base(from, full: full)}#{reference}"
+    else
+      reference
+    end
+  end
+
+  def reference_link_text(from = nil)
+    self.class.reference_prefix + self.title
+  end
+
   def title=(value)
     write_attribute(:title, sanitize_title(value)) if value.present?
   end
@@ -162,6 +192,20 @@ module Timebox
 
   private
 
+  def timebox_format_reference(format = :iid)
+    raise ArgumentError, _('Unknown format') unless [:iid, :name].include?(format)
+
+    if group_timebox? && format == :iid
+      raise ArgumentError, _('Cannot refer to a group %{timebox_type} by an internal id!') % { timebox_type: timebox_name }
+    end
+
+    if format == :name && !name.include?('"')
+      %("#{name}")
+    else
+      iid
+    end
+  end
+
   # Timebox titles must be unique across project and group timeboxes
   def uniqueness_of_title
     if project

app/models/iteration.rb

 # frozen_string_literal: true
 
 class Iteration < ApplicationRecord
-  include Timebox
-
   self.table_name = 'sprints'
 
   attr_accessor :skip_future_date_validation
@@ -15,9 +13,6 @@ class Iteration < ApplicationRecord
 
   include AtomicInternalId
 
-  has_many :issues, foreign_key: 'sprint_id'
-  has_many :merge_requests, foreign_key: 'sprint_id'
-
   belongs_to :project
   belongs_to :group
 
@@ -62,6 +57,14 @@ class Iteration < ApplicationRecord
       else iterations.upcoming
       end
     end
+
+    def reference_prefix
+      '*iteration:'
+    end
+
+    def reference_pattern
+      nil
+    end
   end
 
   def state
@@ -98,3 +101,5 @@ class Iteration < ApplicationRecord
     end
   end
 end
+
+Iteration.prepend_if_ee('EE::Iteration')

app/models/milestone.rb

@@ -2,7 +2,6 @@
 
 class Milestone < ApplicationRecord
   include Sortable
-  include Referable
   include Timebox
   include Milestoneish
   include FromUnion
@@ -122,35 +121,6 @@ class Milestone < ApplicationRecord
     }
   end
 
-  ##
-  # Returns the String necessary to reference a Milestone in Markdown. Group
-  # milestones only support name references, and do not support cross-project
-  # references.
-  #
-  # format - Symbol format to use (default: :iid, optional: :name)
-  #
-  # Examples:
-  #
-  #   Milestone.first.to_reference                           # => "%1"
-  #   Milestone.first.to_reference(format: :name)            # => "%\"goal\""
-  #   Milestone.first.to_reference(cross_namespace_project)  # => "gitlab-org/gitlab-foss%1"
-  #   Milestone.first.to_reference(same_namespace_project)   # => "gitlab-foss%1"
-  #
-  def to_reference(from = nil, format: :name, full: false)
-    format_reference = milestone_format_reference(format)
-    reference = "#{self.class.reference_prefix}#{format_reference}"
-
-    if project
-      "#{project.to_reference_base(from, full: full)}#{reference}"
-    else
-      reference
-    end
-  end
-
-  def reference_link_text(from = nil)
-    self.class.reference_prefix + self.title
-  end
-
   def for_display
     self
   end
@@ -185,20 +155,6 @@ class Milestone < ApplicationRecord
 
   private
 
-  def milestone_format_reference(format = :iid)
-    raise ArgumentError, _('Unknown format') unless [:iid, :name].include?(format)
-
-    if group_milestone? && format == :iid
-      raise ArgumentError, _('Cannot refer to a group milestone by an internal id!')
-    end
-
-    if format == :name && !name.include?('"')
-      %("#{name}")
-    else
-      iid
-    end
-  end
-
   def issues_finder_params
     { project_id: project_id, group_id: group_id, include_subgroups: group_id.present? }.compact
   end

config/application.rb

@@ -316,7 +316,7 @@ module Gitlab
       # conflict with the methods defined in `project_url_helpers`, and we want
       # these methods available in the same places.
       Gitlab::Routing.add_helpers(project_url_helpers)
-      Gitlab::Routing.add_helpers(MilestonesRoutingHelper)
+      Gitlab::Routing.add_helpers(TimeboxesRoutingHelper)
     end
   end
 end

ee/app/helpers/ee/timeboxes_routing_helper.rb

+# frozen_string_literal: true
+
+module EE
+  module TimeboxesRoutingHelper
+    def iteration_path(iteration, *args)
+      if iteration.group_timebox?
+        group_iteration_path(iteration.group, iteration, *args)
+      elsif iteration.project_timebox?
+        # We don't have project iteration routes yet, so for now send users to the project itself
+        project_path(iteration.project, *args)
+      end
+    end
+
+    def iteration_url(iteration, *args)
+      if iteration.group_timebox?
+        group_iteration_url(iteration.group, iteration, *args)
+      elsif iteration.project_timebox?
+        # We don't have project iteration routes yet, so for now send users to the project itself
+        project_url(iteration.project, *args)
+      end
+    end
+  end
+end

ee/app/models/ee/iteration.rb

+# frozen_string_literal: true
+
+module EE
+  module Iteration
+    extend ActiveSupport::Concern
+
+    prepended do
+      include Timebox
+
+      has_many :issues, foreign_key: 'sprint_id'
+      has_many :merge_requests, foreign_key: 'sprint_id'
+    end
+
+    class_methods do
+      def reference_pattern
+        # NOTE: The id pattern only matches when all characters on the expression
+        # are digits, so it will match *iteration:2 but not *iteration:2.1 because that's probably a
+        # iteration name and we want it to be matched as such.
+        @reference_pattern ||= %r{
+        (#{::Project.reference_pattern})?
+        #{::Regexp.escape(reference_prefix)}
+        (?:
+          (?<iteration_id>
+            \d+(?!\S\w)\b # Integer-based iteration id, or
+          ) |
+          (?<iteration_name>
+            [^"\s]+\b |  # String-based single-word iteration title, or
+            "[^"]+"      # String-based multi-word iteration surrounded in quotes
+          )
+        )
+      }x.freeze
+      end
+
+      def link_reference_pattern
+        @link_reference_pattern ||= super("iterations", /(?<iteration>\d+)/)
+      end
+    end
+
+    # Show just the title when we manage to find an iteration, without the reference pattern,
+    # since it's long and unsightly.
+    def reference_link_text(from = nil)
+      self.title
+    end
+
+    private
+
+    def timebox_format_reference(format = :id)
+      raise ::ArgumentError, _('Unknown format') unless [:id, :name].include?(format)
+
+      if format == :name
+        super
+      else
+        id
+      end
+    end
+  end
+end

ee/app/models/ee/system_note_metadata.rb

@@ -9,12 +9,14 @@ module EE
       epic_issue_added issue_added_to_epic epic_issue_removed issue_removed_from_epic
       epic_issue_moved issue_changed_epic epic_date_changed relate_epic unrelate_epic
       vulnerability_confirmed vulnerability_dismissed vulnerability_resolved
+      iteration
     ].freeze
 
     EE_TYPES_WITH_CROSS_REFERENCES = %w[
       relate unrelate
       epic_issue_added issue_added_to_epic epic_issue_removed issue_removed_from_epic
       epic_issue_moved issue_changed_epic relate_epic unrelate_epic
+      iteration
     ].freeze
 
     override :icon_types

ee/app/services/ee/issuable/common_system_notes_service.rb

@@ -7,11 +7,12 @@ module EE
       attr_reader :issuable
 
       override :execute
-      def execute(_issuable, old_labels: [], old_milestone: nil, is_update: true)
+      def execute(issuable, old_labels: [], old_milestone: nil, is_update: true)
         super
 
         ActiveRecord::Base.no_touching do
           handle_weight_change(is_update)
+          handle_iteration_change
 
           if is_update
             handle_date_change_note
@@ -32,6 +33,12 @@ module EE
         end
       end
 
+      def handle_iteration_change
+        return unless issuable.previous_changes.include?('sprint_id')
+
+        ::SystemNoteService.change_iteration(issuable, current_user, issuable.iteration)
+      end
+
       def handle_weight_change(is_update)
         return unless issuable.previous_changes.include?('weight')
 

ee/app/services/ee/system_note_service.rb

@@ -44,6 +44,10 @@ module EE
       epics_service(epic, user).issue_epic_change(issue)
     end
 
+    def change_iteration(noteable, author, iteration)
+      issuables_service(noteable, noteable.project, author).change_iteration(iteration)
+    end
+
     # Called when the merge request is approved by user
     #
     # noteable - Noteable object

ee/app/services/ee/system_notes/issuables_service.rb

@@ -74,6 +74,23 @@ module EE
 
         create_note(NoteSummary.new(noteable, project, author, body, action: 'published'))
       end
+
+      # Called when the iteration of a Noteable is changed
+      #
+      # iteration - Iteration being assigned, or nil
+      #
+      # Example Note text:
+      #
+      #   "removed iteration"
+      #
+      #   "changed iteration to 7.11"
+      #
+      # Returns the created Note object
+      def change_iteration(iteration)
+        body = iteration.nil? ? 'removed iteration' : "changed iteration to #{iteration.to_reference(project, format: :id)}"
+
+        create_note(NoteSummary.new(noteable, project, author, body, action: 'iteration'))
+      end
     end
   end
 end

ee/lib/ee/banzai/filter/iteration_reference_filter.rb

+# frozen_string_literal: true
+
+module EE
+  module Banzai
+    module Filter
+      # HTML filter that replaces iteration references with links.
+      module IterationReferenceFilter
+        include ::Gitlab::Utils::StrongMemoize
+
+        def find_object(parent, id)
+          return unless valid_context?(parent)
+
+          find_iteration_with_finder(parent, id: id)
+        end
+
+        def valid_context?(parent)
+          group_context?(parent) || project_context?(parent)
+        end
+
+        def group_context?(parent)
+          strong_memoize(:group_context) do
+            parent.is_a?(Group)
+          end
+        end
+
+        def project_context?(parent)
+          strong_memoize(:project_context) do
+            parent.is_a?(Project)
+          end
+        end
+
+        def references_in(text, pattern = ::Iteration.reference_pattern)
+          # We'll handle here the references that follow the `reference_pattern`.
+          # Other patterns (for example, the link pattern) are handled by the
+          # default implementation.
+          return super(text, pattern) if pattern != ::Iteration.reference_pattern
+
+          iterations = {}
+          unescaped_html = unescape_html_entities(text).gsub(pattern) do |match|
+            iteration = find_iteration($~[:project], $~[:namespace], $~[:iteration_id], $~[:iteration_name])
+
+            if iteration
+              iterations[iteration.id] = yield match, iteration.id, $~[:project], $~[:namespace], $~
+              "#{::Banzai::Filter::AbstractReferenceFilter::REFERENCE_PLACEHOLDER}#{iteration.id}"
+            else
+              match
+            end
+          end
+
+          return text if iterations.empty?
+
+          escape_with_placeholders(unescaped_html, iterations)
+        end
+
+        def find_iteration(project_ref, namespace_ref, iteration_id, iteration_name)
+          project_path = full_project_path(namespace_ref, project_ref)
+
+          # Returns group if project is not found by path
+          parent = parent_from_ref(project_path)
+
+          return unless parent
+
+          iteration_params = iteration_params(iteration_id, iteration_name)
+
+          find_iteration_with_finder(parent, iteration_params)
+        end
+
+        def iteration_params(id, name)
+          if name
+            { name: name.tr('"', '') }
+          else
+            { id: id.to_i }
+          end
+        end
+
+        # rubocop:disable CodeReuse/ActiveRecord
+        def find_iteration_with_finder(parent, params)
+          finder_params = iteration_finder_params(parent)
+
+          ::IterationsFinder.new(finder_params).find_by(params)
+        end
+        # rubocop:enable CodeReuse/ActiveRecord
+
+        def iteration_finder_params(parent)
+          { order: nil, state: 'all' }.tap do |params|
+            params[:project_ids] = parent.id if project_context?(parent)
+
+            params[:group_ids] = self_and_ancestors_ids(parent)
+          end
+        end
+
+        def self_and_ancestors_ids(parent)
+          if group_context?(parent)
+            parent.self_and_ancestors.select(:id)
+          elsif project_context?(parent)
+            parent.group&.self_and_ancestors&.select(:id)
+          end
+        end
+
+        def url_for_object(iteration, _parent)
+          ::Gitlab::Routing
+              .url_helpers
+              .iteration_url(iteration, only_path: context[:only_path])
+        end
+
+        def object_link_text(object, matches)
+          iteration_link = escape_once(super)
+          reference = object.project&.to_reference_base(project)
+
+          if reference.present?
+            "#{iteration_link} <i>in #{reference}</i>".html_safe
+          else
+            iteration_link
+          end
+        end
+
+        def object_link_title(_object, _matches)
+          'Iteration'
+        end
+      end
+    end
+  end
+end

ee/lib/ee/banzai/pipeline/gfm_pipeline.rb

@@ -18,6 +18,7 @@ module EE
           def reference_filters
             [
               ::Banzai::Filter::EpicReferenceFilter,
+              ::Banzai::Filter::IterationReferenceFilter,
               *super
             ]
           end

ee/lib/ee/banzai/pipeline/single_line_pipeline.rb

@@ -10,6 +10,7 @@ module EE
           def reference_filters
             [
               ::Banzai::Filter::EpicReferenceFilter,
+              ::Banzai::Filter::IterationReferenceFilter,
               *super
             ]
           end

ee/lib/ee/banzai/reference_parser/iteration_parser.rb

+# frozen_string_literal: true
+
+module EE
+  module Banzai
+    module ReferenceParser
+      module IterationParser
+        private
+
+        def can_read_reference?(user, ref_project, node)
+          can?(user, :read_iteration, ref_project)
+        end
+      end
+    end
+  end
+end

ee/spec/lib/banzai/reference_parser/iteration_parser_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Banzai::ReferenceParser::IterationParser do
+  include ReferenceParserHelpers
+
+  let(:project) { create(:project, :public) }
+  let(:user) { create(:user) }
+  let(:iteration) { create(:iteration, project: project) }
+  subject { described_class.new(Banzai::RenderContext.new(project, user)) }
+
+  let(:link) { empty_html_link }
+
+  describe '#nodes_visible_to_user' do
+    context 'when the link has a data-iteration attribute' do
+      before do
+        link['data-iteration'] = iteration.id.to_s
+      end
+
+      it_behaves_like "referenced feature visibility", "issues", "merge_requests"
+    end
+  end
+
+  describe '#referenced_by' do
+    describe 'when the link has a data-iteration attribute' do
+      context 'using an existing iteration ID' do
+        it 'returns an Array of iterations' do
+          link['data-iteration'] = iteration.id.to_s
+
+          expect(subject.referenced_by([link])).to eq([iteration])
+        end
+      end
+
+      context 'using a non-existing iteration ID' do
+        it 'returns an empty Array' do
+          link['data-iteration'] = ''
+
+          expect(subject.referenced_by([link])).to eq([])
+        end
+      end
+    end
+  end
+end

ee/spec/models/ee/iteration_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Iteration do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:group) { create(:group) }
+
+  it_behaves_like 'a timebox', :iteration do
+    let(:timebox_table_name) { described_class.table_name.to_sym }
+  end
+end

ee/spec/requests/api/graphql/mutations/iterations/create_spec.rb

@@ -79,7 +79,7 @@ describe 'Creating an Iteration' do
         let(:attributes) { { title: '' } }
 
         it_behaves_like 'a mutation that returns errors in the response',
-                        errors: ["Title can't be blank", "Start date can't be blank", "Due date can't be blank"]
+                        errors: ["Start date can't be blank", "Due date can't be blank", "Title can't be blank"]
 
         it 'does not create the iteration' do
           expect { post_graphql_mutation(mutation, current_user: current_user) }.not_to change(Iteration, :count)

ee/spec/services/ee/issues/update_service_spec.rb

@@ -71,6 +71,33 @@ describe Issues::UpdateService do
       end
     end
 
+    context 'assigning iteration' do
+      before do
+        stub_licensed_features(iterations: true)
+        group.add_maintainer(user)
+      end
+
+      context 'group iterations' do
+        it 'creates a system note' do
+          group_iteration = create(:iteration, group: group)
+
+          expect do
+            update_issue(iteration: group_iteration)
+          end.to change { Note.system.count }.by(1)
+        end
+      end
+
+      context 'project iterations' do
+        it 'creates a system note' do
+          project_iteration = create(:iteration, project: project)
+
+          expect do
+            update_issue(iteration: project_iteration)
+          end.to change { Note.system.count }.by(1)
+        end
+      end
+    end
+
     context 'assigning epic' do
       before do
         stub_licensed_features(epics: true)

ee/spec/services/ee/system_notes/issuables_service_spec.rb

@@ -118,4 +118,58 @@ describe ::SystemNotes::IssuablesService do
       expect(subject.note).to eq 'published this issue to the status page'
     end
   end
+
+  describe '#change_iteration' do
+    subject { service.change_iteration(iteration) }
+
+    context 'for a project iteration' do
+      let(:iteration) { create(:iteration, project: project) }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'iteration' }
+      end
+
+      it_behaves_like 'a note with overridable created_at'
+
+      context 'when iteration added' do
+        it 'sets the note text' do
+          reference = iteration.to_reference(format: :id)
+
+          expect(subject.note).to eq "changed iteration to #{reference}"
+        end
+      end
+
+      context 'when iteration removed' do
+        let(:iteration) { nil }
+
+        it 'sets the note text' do
+          expect(subject.note).to eq 'removed iteration'
+        end
+      end
+    end
+
+    context 'for a group iteration' do
+      let(:iteration) { create(:iteration, group: group) }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'iteration' }
+      end
+
+      it_behaves_like 'a note with overridable created_at'
+
+      context 'when iteration added' do
+        it 'sets the note text to use the iteration id' do
+          expect(subject.note).to eq "changed iteration to #{iteration.to_reference(format: :id)}"
+        end
+      end
+
+      context 'when iteration removed' do
+        let(:iteration) { nil }
+
+        it 'sets the note text' do
+          expect(subject.note).to eq 'removed iteration'
+        end
+      end
+    end
+  end
 end

ee/spec/services/system_note_service_spec.rb

@@ -225,4 +225,14 @@ describe SystemNoteService do
       described_class.publish_issue_to_status_page(noteable, project, author)
     end
   end
+
+  describe '.change_iteration' do
+    it 'calls IssuablesService' do
+      expect_next_instance_of(::SystemNotes::IssuablesService) do |service|
+        expect(service).to receive(:change_iteration)
+      end
+
+      described_class.change_iteration(noteable, author, nil)
+    end
+  end
 end

lib/banzai/filter/iteration_reference_filter.rb

+# frozen_string_literal: true
+
+module Banzai
+  module Filter
+    # The actual filter is implemented in the EE mixin
+    class IterationReferenceFilter < AbstractReferenceFilter
+      self.reference_type = :iteration
+
+      def self.object_class
+        Iteration
+      end
+    end
+  end
+end
+
+Banzai::Filter::IterationReferenceFilter.prepend_if_ee('EE::Banzai::Filter::IterationReferenceFilter')

lib/banzai/reference_parser/iteration_parser.rb

+# frozen_string_literal: true
+
+module Banzai
+  module ReferenceParser
+    # The actual parser is implemented in the EE mixin
+    class IterationParser < BaseParser
+      self.reference_type = :iteration
+
+      def references_relation
+        Iteration
+      end
+
+      private
+
+      def can_read_reference?(_user, _ref_project, _node)
+        false
+      end
+    end
+  end
+end
+
+Banzai::ReferenceParser::IterationParser.prepend_if_ee('::EE::Banzai::ReferenceParser::IterationParser')

lib/gitlab/reference_extractor.rb

@@ -4,7 +4,7 @@ module Gitlab
   # Extract possible GFM references from an arbitrary String for further processing.
   class ReferenceExtractor < Banzai::ReferenceExtractor
     REFERABLES = %i(user issue label milestone mentioned_user mentioned_group mentioned_project
-                    merge_request snippet commit commit_range directly_addressed_user epic).freeze
+                    merge_request snippet commit commit_range directly_addressed_user epic iteration).freeze
     attr_accessor :project, :current_user, :author
     # This counter is increased by a number of references filtered out by
     # banzai reference exctractor. Note that this counter is stateful and

locale/gitlab.pot

@@ -3761,7 +3761,7 @@ msgstr ""
 msgid "Cannot promote issue due to insufficient permissions."
 msgstr ""
 
-msgid "Cannot refer to a group milestone by an internal id!"
+msgid "Cannot refer to a group %{timebox_type} by an internal id!"
 msgstr ""
 
 msgid "Cannot set confidential epic for not-confidential issue"

spec/helpers/milestones_routing_helper_spec.rb → spec/helpers/timeboxes_routing_helper_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe MilestonesRoutingHelper do
+describe TimeboxesRoutingHelper do
   let(:project) { build_stubbed(:project) }
   let(:group) { build_stubbed(:group) }
 

spec/lib/gitlab/reference_extractor_spec.rb

@@ -296,7 +296,7 @@ describe Gitlab::ReferenceExtractor do
     end
 
     it 'returns all supported prefixes' do
-      expect(prefixes.keys.uniq).to match_array(%w(@ # ~ % ! $ &))
+      expect(prefixes.keys.uniq).to match_array(%w(@ # ~ % ! $ & *iteration:))
     end
 
     it 'does not allow one prefix for multiple referables if not allowed specificly' do

spec/models/iteration_spec.rb

@@ -6,10 +6,6 @@ describe Iteration do
   let_it_be(:project) { create(:project) }
   let_it_be(:group) { create(:group) }
 
-  it_behaves_like 'a timebox', :iteration do
-    let(:timebox_table_name) { described_class.table_name.to_sym }
-  end
-
   describe "#iid" do
     it "is properly scoped on project and group" do
       iteration1 = create(:iteration, project: project)