Merge branch 'psi-iteration-iid-fix' into 'master'

Load correct iteration on iteration report

See merge request gitlab-org/gitlab!34842

app/models/application_record.rb

@@ -13,6 +13,10 @@ class ApplicationRecord < ActiveRecord::Base
     where(id: ids)
   end
 
+  def self.iid_in(iids)
+    where(iid: iids)
+  end
+
   def self.id_not_in(ids)
     where.not(id: ids)
   end

app/models/project.rb

@@ -602,6 +602,14 @@ class Project < ApplicationRecord
     end
   end
 
+  def self.projects_user_can(projects, user, action)
+    projects = where(id: projects)
+
+    DeclarativePolicy.user_scope do
+      projects.select { |project| Ability.allowed?(user, action, project) }
+    end
+  end
+
   # This scope returns projects where user has access to both the project and the feature.
   def self.filter_by_feature_visibility(feature, user)
     with_feature_available_for_user(feature, user)

doc/api/graphql/reference/gitlab_schema.graphql

@@ -5094,6 +5094,11 @@ type Group {
     """
     id: ID
 
+    """
+    The internal ID of the Iteration to look up
+    """
+    iid: ID
+
     """
     Returns the last _n_ elements from the list.
     """
@@ -6301,6 +6306,11 @@ type Iteration {
   """
   id: ID!
 
+  """
+  Internal ID of the iteration
+  """
+  iid: ID!
+
   """
   Timestamp of the iteration start date
   """

doc/api/graphql/reference/gitlab_schema.json

@@ -14100,6 +14100,16 @@
                   },
                   "defaultValue": null
                 },
+                {
+                  "name": "iid",
+                  "description": "The internal ID of the Iteration to look up",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "ID",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                },
                 {
                   "name": "after",
                   "description": "Returns the elements in the list that come after the specified cursor.",
@@ -17329,6 +17339,24 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "iid",
+              "description": "Internal ID of the iteration",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "NON_NULL",
+                "name": null,
+                "ofType": {
+                  "kind": "SCALAR",
+                  "name": "ID",
+                  "ofType": null
+                }
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "startDate",
               "description": "Timestamp of the iteration start date",

doc/api/graphql/reference/index.md

@@ -946,6 +946,7 @@ Represents an iteration object.
 | `description` | String | Description of the iteration |
 | `dueDate` | Time | Timestamp of the iteration due date |
 | `id` | ID! | ID of the iteration |
+| `iid` | ID! | Internal ID of the iteration |
 | `startDate` | Time | Timestamp of the iteration start date |
 | `state` | IterationState! | State of the iteration |
 | `title` | String! | Title of the iteration |

ee/app/assets/javascripts/iterations/components/iteration_report.vue

@@ -10,7 +10,6 @@ import {
 } from '@gitlab/ui';
 import { formatDate } from '~/lib/utils/datetime_utility';
 import { __ } from '~/locale';
-import { getIdFromGraphQLId } from '~/graphql_shared/utils';
 import IterationForm from './iteration_form.vue';
 import query from '../queries/group_iteration.query.graphql';
 
@@ -37,7 +36,7 @@ export default {
       variables() {
         return {
           groupPath: this.groupPath,
-          id: getIdFromGraphQLId(this.iterationId),
+          iid: this.iterationIid,
         };
       },
       update(data) {
@@ -57,7 +56,7 @@ export default {
       type: String,
       required: true,
     },
-    iterationId: {
+    iterationIid: {
       type: String,
       required: true,
     },

ee/app/assets/javascripts/iterations/index.js

@@ -51,7 +51,7 @@ export function initIterationForm() {
 export function initIterationReport() {
   const el = document.querySelector('.js-iteration');
 
-  const { groupPath, iterationId, editIterationPath } = el.dataset;
+  const { groupPath, iterationIid, editIterationPath } = el.dataset;
   const canEdit = parseBoolean(el.dataset.canEdit);
 
   return new Vue({
@@ -61,7 +61,7 @@ export function initIterationReport() {
       return createElement(IterationReport, {
         props: {
           groupPath,
-          iterationId,
+          iterationIid,
           canEdit,
           editIterationPath,
         },

ee/app/assets/javascripts/iterations/queries/group_iteration.query.graphql

-query GroupIteration($groupPath: ID!, $id: ID!) {
+query GroupIteration($groupPath: ID!, $iid: ID!) {
   group(fullPath: $groupPath) {
-    iterations(id: $id, first: 1) {
+    iterations(iid: $iid, first: 1) {
       nodes {
         title
         state
+        iid
         id
         description
         webPath

ee/app/finders/iterations_finder.rb

@@ -13,15 +13,19 @@ class IterationsFinder
   include FinderMethods
   include TimeFrameFilter
 
-  attr_reader :params
+  attr_reader :params, :current_user
 
-  def initialize(params = {})
+  def initialize(current_user, params = {})
     @params = params
+    @current_user = current_user
   end
 
   def execute
+    filter_permissions
+
     items = Iteration.all
     items = by_id(items)
+    items = by_iid(items)
     items = by_groups_and_projects(items)
     items = by_title(items)
     items = by_search_title(items)
@@ -33,6 +37,31 @@ class IterationsFinder
 
   private
 
+  def filter_permissions
+    filter_allowed_projects
+    filter_allowed_groups
+
+    # Only allow either one project_id or one group_id when filtering by `iid`
+    if params[:iid] && params.slice(:project_ids, :group_ids).keys.count > 1
+      raise ArgumentError, 'You can specify only one scope if you use iid filter'
+    end
+  end
+
+  def filter_allowed_projects
+    return unless params[:project_ids].present?
+
+    projects = Project.id_in(params[:project_ids])
+    params[:project_ids] = Project.projects_user_can(projects, current_user, :read_iteration)
+  end
+
+  def filter_allowed_groups
+    return unless params[:group_ids].present?
+
+    groups = Group.id_in(params[:group_ids])
+
+    params[:group_ids] = Group.groups_user_can(groups, current_user, :read_iteration)
+  end
+
   def by_groups_and_projects(items)
     items.for_projects_and_groups(params[:project_ids], params[:group_ids])
   end
@@ -45,6 +74,10 @@ class IterationsFinder
     end
   end
 
+  def by_iid(items)
+    params[:iid].present? ? items.iid_in(params[:iid]) : items
+  end
+
   def by_title(items)
     if params[:title]
       items.with_title(params[:title])

ee/app/graphql/resolvers/iterations_resolver.rb

@@ -14,6 +14,9 @@ module Resolvers
     argument :id, GraphQL::ID_TYPE,
              required: false,
              description: 'The ID of the Iteration to look up'
+    argument :iid, GraphQL::ID_TYPE,
+             required: false,
+             description: 'The internal ID of the Iteration to look up'
 
     type Types::IterationType, null: true
 
@@ -22,7 +25,7 @@ module Resolvers
 
       authorize!
 
-      iterations = IterationsFinder.new(iterations_finder_params(args)).execute
+      iterations = IterationsFinder.new(context[:current_user], iterations_finder_params(args)).execute
 
       Gitlab::Graphql::Pagination::OffsetActiveRecordRelationConnection.new(iterations)
     end
@@ -32,6 +35,7 @@ module Resolvers
     def iterations_finder_params(args)
       {
         id: args[:id],
+        iid: args[:iid],
         state: args[:state] || 'all',
         start_date: args[:start_date],
         end_date: args[:end_date],

ee/app/graphql/types/iteration_type.rb

@@ -12,6 +12,9 @@ module Types
     field :id, GraphQL::ID_TYPE, null: false,
           description: 'ID of the iteration'
 
+    field :iid, GraphQL::ID_TYPE, null: false,
+          description: 'Internal ID of the iteration'
+
     field :title, GraphQL::STRING_TYPE, null: false,
           description: 'Title of the iteration'
 

ee/app/models/ee/group.rb

@@ -142,7 +142,7 @@ module EE
     end
 
     class_methods do
-      def groups_user_can_read_epics(groups, user, same_root: false)
+      def groups_user_can(groups, user, action, same_root: false)
         groups = ::Gitlab::GroupPlansPreloader.new.preload(groups)
 
         # if we are sure that all groups have the same root group, we can
@@ -152,10 +152,14 @@ module EE
         preset_root_ancestor_for(groups) if same_root
 
         DeclarativePolicy.user_scope do
-          groups.select { |group| Ability.allowed?(user, :read_epic, group) }
+          groups.select { |group| Ability.allowed?(user, action, group) }
         end
       end
 
+      def groups_user_can_read_epics(groups, user, same_root: false)
+        groups_user_can(groups, user, :read_epic, same_root: same_root)
+      end
+
       def preset_root_ancestor_for(groups)
         return groups if groups.size < 2
 

ee/app/views/groups/iterations/show.html.haml

@@ -3,4 +3,4 @@
 - page_title _("Iterations")
 
 - if Feature.enabled?(:group_iterations, @group)
-  .js-iteration{ data: { group_path: @group.full_path, iteration_id: params[:id], can_edit: can?(current_user, :admin_iteration, @group).to_s, preview_markdown_path: preview_markdown_path(@group) } }
+  .js-iteration{ data: { group_path: @group.full_path, iteration_iid: params[:id], can_edit: can?(current_user, :admin_iteration, @group).to_s, preview_markdown_path: preview_markdown_path(@group) } }

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

@@ -77,7 +77,7 @@ module EE
         def find_iteration_with_finder(parent, params)
           finder_params = iteration_finder_params(parent)
 
-          ::IterationsFinder.new(finder_params).find_by(params)
+          ::IterationsFinder.new(context[:current_user], finder_params).find_by(params)
         end
         # rubocop:enable CodeReuse/ActiveRecord
 

ee/lib/ee/gitlab/quick_actions/issue_actions.rb

@@ -130,9 +130,9 @@ module EE
           end
 
           def find_iterations(project, params = {})
-            group_ids = project.group.self_and_ancestors.select(:id) if project.group
+            group_ids = project.group.self_and_ancestors.map(&:id) if project.group
 
-            ::IterationsFinder.new(params.merge(project_ids: [project.id], group_ids: group_ids)).execute
+            ::IterationsFinder.new(current_user, params.merge(project_ids: [project.id], group_ids: group_ids)).execute
           end
 
           desc _('Publish to status page')

ee/lib/gitlab/group_plans_preloader.rb

@@ -16,7 +16,6 @@ module Gitlab
     # rubocop: disable CodeReuse/ActiveRecord
     def preload(groups)
       groups_and_ancestors = groups_and_ancestors_for(groups)
-
       # A Hash mapping group IDs to their corresponding Group instances.
       groups_map = groups_and_ancestors.each_with_object({}) do |group, hash|
         hash[group.id] = group

ee/spec/features/groups/iterations/user_views_iteration_spec.rb

@@ -5,25 +5,24 @@ require 'spec_helper'
 RSpec.describe 'User views iteration' do
   let_it_be(:now) { Time.now }
   let_it_be(:group) { create(:group) }
-  let_it_be(:user) { create(:group_member, :maintainer, user: create(:user), group: group ).user }
   let_it_be(:iteration) { create(:iteration, :skip_future_date_validation, group: group, start_date: now - 1.day, due_date: now) }
 
-  around do |example|
-    Timecop.freeze { example.run }
-  end
-
-  before do
-    sign_in(user)
-  end
-
-  context 'view an iteration', :js, quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/222915' do
+  context 'with license' do
     before do
-      visit group_iteration_path(iteration.group, iteration)
+      stub_licensed_features(iterations: true)
     end
 
-    it 'shows iteration info and dates' do
-      expect(page).to have_content(iteration.title)
-      expect(page).to have_content(iteration.description)
+    context 'view an iteration', :js do
+      before do
+        visit group_iteration_path(iteration.group, iteration)
+      end
+
+      it 'shows iteration info and dates' do
+        expect(page).to have_content(iteration.title)
+        expect(page).to have_content(iteration.description)
+        expect(page).to have_content(iteration.start_date.strftime('%b %-d, %Y'))
+        expect(page).to have_content(iteration.due_date.strftime('%b %-d, %Y'))
+      end
     end
   end
 end

ee/spec/finders/iterations_finder_spec.rb

@@ -4,127 +4,160 @@ require 'spec_helper'
 
 RSpec.describe IterationsFinder do
   let(:now) { Time.now }
-  let_it_be(:group) { create(:group) }
+  let_it_be(:group) { create(:group, :private) }
   let_it_be(:project_1) { create(:project, namespace: group) }
   let_it_be(:project_2) { create(:project, namespace: group) }
+  let_it_be(:user) { create(:user) }
   let!(:started_group_iteration) { create(:started_iteration, :skip_future_date_validation, group: group, title: 'one test', start_date: now - 1.day, due_date: now) }
   let!(:upcoming_group_iteration) { create(:iteration, group: group, start_date: 1.day.from_now, due_date: 2.days.from_now) }
   let!(:iteration_from_project_1) { create(:started_iteration, project: project_1, start_date: 2.days.from_now, due_date: 3.days.from_now) }
   let!(:iteration_from_project_2) { create(:started_iteration, project: project_2, start_date: 4.days.from_now, due_date: 5.days.from_now) }
   let(:project_ids) { [project_1.id, project_2.id] }
 
-  subject { described_class.new(params).execute }
+  subject { described_class.new(user, params).execute }
 
-  context 'iterations for projects' do
-    let(:params) { { project_ids: project_ids, state: 'all' } }
+  context 'without permissions' do
+    context 'groups and projects' do
+      let(:params) { { project_ids: project_ids, group_ids: group.id, state: 'all' } }
 
-    it 'returns iterations for projects' do
-      expect(subject).to contain_exactly(iteration_from_project_1, iteration_from_project_2)
+      it 'returns iterations for groups and projects' do
+        expect(subject).to be_empty
+      end
     end
   end
 
-  context 'iterations for groups' do
-    let(:params) { { group_ids: group.id, state: 'all' } }
-
-    it 'returns iterations for groups' do
-      expect(subject).to contain_exactly(started_group_iteration, upcoming_group_iteration)
+  context 'with permissions' do
+    before do
+      group.add_reporter(user)
+      project_1.add_reporter(user)
+      project_2.add_reporter(user)
     end
-  end
 
-  context 'iterations for groups and project' do
-    let(:params) { { project_ids: project_ids, group_ids: group.id, state: 'all' } }
+    context 'iterations for projects' do
+      let(:params) { { project_ids: project_ids, state: 'all' } }
 
-    it 'returns iterations for groups and projects' do
-      expect(subject).to contain_exactly(started_group_iteration, upcoming_group_iteration, iteration_from_project_1, iteration_from_project_2)
+      it 'returns iterations for projects' do
+        expect(subject).to contain_exactly(iteration_from_project_1, iteration_from_project_2)
+      end
     end
 
-    it 'orders iterations by due date' do
-      iteration = create(:iteration, :skip_future_date_validation, group: group, start_date: now - 3.days, due_date: now - 2.days)
+    context 'iterations for groups' do
+      let(:params) { { group_ids: group.id, state: 'all' } }
 
-      expect(subject.first).to eq(iteration)
-      expect(subject.second).to eq(started_group_iteration)
-      expect(subject.third).to eq(upcoming_group_iteration)
+      it 'returns iterations for groups' do
+        expect(subject).to contain_exactly(started_group_iteration, upcoming_group_iteration)
+      end
     end
-  end
 
-  context 'with filters' do
-    let(:params) do
-      {
-        project_ids: project_ids,
-        group_ids: group.id,
-        state: 'all'
-      }
-    end
+    context 'iterations for groups and project' do
+      let(:params) { { project_ids: project_ids, group_ids: group.id, state: 'all' } }
 
-    before do
-      started_group_iteration.close
-      iteration_from_project_1.close
-    end
+      it 'returns iterations for groups and projects' do
+        expect(subject).to contain_exactly(started_group_iteration, upcoming_group_iteration, iteration_from_project_1, iteration_from_project_2)
+      end
 
-    it 'filters by started state' do
-      params[:state] = 'started'
+      it 'orders iterations by due date' do
+        iteration = create(:iteration, :skip_future_date_validation, group: group, start_date: now - 3.days, due_date: now - 2.days)
 
-      expect(subject).to contain_exactly(iteration_from_project_2)
+        expect(subject.first).to eq(iteration)
+        expect(subject.second).to eq(started_group_iteration)
+        expect(subject.third).to eq(upcoming_group_iteration)
+      end
     end
 
-    it 'filters by opened state' do
-      params[:state] = 'opened'
+    context 'with filters' do
+      let(:params) do
+        {
+          project_ids: project_ids,
+          group_ids: group.id,
+          state: 'all'
+        }
+      end
 
-      expect(subject).to contain_exactly(upcoming_group_iteration, iteration_from_project_2)
-    end
+      before do
+        started_group_iteration.close
+        iteration_from_project_1.close
+      end
 
-    it 'filters by closed state' do
-      params[:state] = 'closed'
+      it 'filters by started state' do
+        params[:state] = 'started'
 
-      expect(subject).to contain_exactly(started_group_iteration, iteration_from_project_1)
-    end
+        expect(subject).to contain_exactly(iteration_from_project_2)
+      end
 
-    it 'filters by title' do
-      params[:title] = 'one test'
+      it 'filters by opened state' do
+        params[:state] = 'opened'
 
-      expect(subject.to_a).to contain_exactly(started_group_iteration)
-    end
+        expect(subject).to contain_exactly(upcoming_group_iteration, iteration_from_project_2)
+      end
 
-    it 'filters by search_title' do
-      params[:search_title] = 'one t'
+      it 'filters by closed state' do
+        params[:state] = 'closed'
 
-      expect(subject.to_a).to contain_exactly(started_group_iteration)
-    end
+        expect(subject).to contain_exactly(started_group_iteration, iteration_from_project_1)
+      end
 
-    it 'filters by ID' do
-      params[:id] = iteration_from_project_1.id
+      it 'filters by title' do
+        params[:title] = 'one test'
 
-      expect(subject).to contain_exactly(iteration_from_project_1)
-    end
+        expect(subject.to_a).to contain_exactly(started_group_iteration)
+      end
 
-    context 'by timeframe' do
-      it 'returns iterations with start_date and due_date between timeframe' do
-        params.merge!(start_date: now - 1.day, end_date: 3.days.from_now)
+      it 'filters by search_title' do
+        params[:search_title] = 'one t'
 
-        expect(subject).to match_array([started_group_iteration, upcoming_group_iteration, iteration_from_project_1])
+        expect(subject.to_a).to contain_exactly(started_group_iteration)
       end
 
-      it 'returns iterations which start before the timeframe' do
-        iteration = create(:iteration, :skip_future_date_validation, project: project_2, start_date: now - 5.days, due_date: now - 3.days)
-        params.merge!(start_date: now - 3.days, end_date: now - 2.days)
+      it 'filters by ID' do
+        params[:id] = iteration_from_project_1.id
 
-        expect(subject).to match_array([iteration])
+        expect(subject).to contain_exactly(iteration_from_project_1)
       end
 
-      it 'returns iterations which end after the timeframe' do
-        iteration = create(:iteration, project: project_2, start_date: 6.days.from_now, due_date: 2.weeks.from_now)
-        params.merge!(start_date: 6.days.from_now, end_date: 7.days.from_now)
+      context 'by timeframe' do
+        it 'returns iterations with start_date and due_date between timeframe' do
+          params.merge!(start_date: now - 1.day, end_date: 3.days.from_now)
+
+          expect(subject).to match_array([started_group_iteration, upcoming_group_iteration, iteration_from_project_1])
+        end
+
+        it 'returns iterations which start before the timeframe' do
+          iteration = create(:iteration, :skip_future_date_validation, project: project_2, start_date: now - 5.days, due_date: now - 3.days)
+          params.merge!(start_date: now - 3.days, end_date: now - 2.days)
+
+          expect(subject).to match_array([iteration])
+        end
 
-        expect(subject).to match_array([iteration])
+        it 'returns iterations which end after the timeframe' do
+          iteration = create(:iteration, project: project_2, start_date: 6.days.from_now, due_date: 2.weeks.from_now)
+          params.merge!(start_date: 6.days.from_now, end_date: 7.days.from_now)
+
+          expect(subject).to match_array([iteration])
+        end
       end
     end
-  end
 
-  describe '#find_by' do
-    it 'finds a single iteration' do
-      finder = described_class.new(project_ids: [project_1.id], state: 'all')
+    describe 'iid' do
+      let(:params) do
+        {
+          project_ids: project_ids,
+          group_ids: group.id,
+          iid: iteration_from_project_1.iid
+        }
+      end
+
+      it 'only accepts one of project_id or group_id' do
+        expect { subject }.to raise_error(ArgumentError, 'You can specify only one scope if you use iid filter')
+      end
+    end
 
-      expect(finder.find_by(iid: iteration_from_project_1.iid)).to eq(iteration_from_project_1)
+    describe '#find_by' do
+      it 'finds a single iteration' do
+        finder = described_class.new(user, project_ids: [project_1.id], state: 'all')
+
+        expect(finder.find_by(iid: iteration_from_project_1.iid)).to eq(iteration_from_project_1)
+      end
     end
   end
 end

ee/spec/frontend/iterations/components/iteration_report_spec.js

@@ -6,7 +6,7 @@ describe('Iterations tabs', () => {
   let wrapper;
   const defaultProps = {
     groupPath: 'gitlab-org',
-    iterationId: '3',
+    iterationIid: '3',
   };
 
   const findTopbar = () => wrapper.find({ ref: 'topbar' });

ee/spec/graphql/resolvers/iterations_resolver_spec.rb

@@ -30,9 +30,9 @@ RSpec.describe Resolvers::IterationsResolver do
 
       context 'without parameters' do
         it 'calls IterationsFinder to retrieve all iterations' do
-          params = { id: nil, group_ids: group.id, state: 'all', start_date: nil, end_date: nil, search_title: nil }
+          params = { id: nil, iid: nil, group_ids: group.id, state: 'all', start_date: nil, end_date: nil, search_title: nil }
 
-          expect(IterationsFinder).to receive(:new).with(params).and_call_original
+          expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
 
           resolve_group_iterations
         end
@@ -44,11 +44,12 @@ RSpec.describe Resolvers::IterationsResolver do
           end_date = start_date + 1.hour
           search = 'wow'
           id = 1
-          params = { id: id, group_ids: group.id, state: 'closed', start_date: start_date, end_date: end_date, search_title: search }
+          iid = 2
+          params = { id: id, iid: iid, group_ids: group.id, state: 'closed', start_date: start_date, end_date: end_date, search_title: search }
 
-          expect(IterationsFinder).to receive(:new).with(params).and_call_original
+          expect(IterationsFinder).to receive(:new).with(current_user, params).and_call_original
 
-          resolve_group_iterations(start_date: start_date, end_date: end_date, state: 'closed', title: search, id: id)
+          resolve_group_iterations(start_date: start_date, end_date: end_date, state: 'closed', title: search, id: id, iid: iid)
         end
       end
 

ee/spec/services/quick_actions/interpret_service_spec.rb

@@ -221,11 +221,17 @@ RSpec.describe QuickActions::InterpretService do
         end
 
         context 'when iteration exists' do
-          it 'assigns an iteration to an issue' do
-            _, updates, message = service.execute(content, issue)
+          context 'with permissions' do
+            before do
+              group.add_developer(current_user)
+            end
 
-            expect(updates).to eq(iteration: iteration)
-            expect(message).to eq("Set the iteration to #{iteration.to_reference}.")
+            it 'assigns an iteration to an issue' do
+              _, updates, message = service.execute(content, issue)
+
+              expect(updates).to eq(iteration: iteration)
+              expect(message).to eq("Set the iteration to #{iteration.to_reference}.")
+            end
           end
 
           context 'when the user does not have enough permissions' do