diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb index 6b1d418fc9a07f11e55088923883786cfe584b03..5c10d7bc26105b7c18c99cab480f78cbff9a7220 100644 --- a/app/controllers/groups/milestones_controller.rb +++ b/app/controllers/groups/milestones_controller.rb @@ -2,13 +2,13 @@ class Groups::MilestonesController < Groups::ApplicationController include MilestoneActions before_action :group_projects - before_action :milestone, only: [:show, :update, :merge_requests, :participants, :labels] - before_action :authorize_admin_milestones!, only: [:new, :create, :update] + before_action :milestone, only: [:edit, :show, :update, :merge_requests, :participants, :labels] + before_action :authorize_admin_milestones!, only: [:edit, :new, :create, :update] def index respond_to do |format| format.html do - @milestone_states = GlobalMilestone.states_count(@projects) + @milestone_states = GlobalMilestone.states_count(group_projects, group) @milestones = Kaminari.paginate_array(milestones).page(params[:page]) end format.json do @@ -22,49 +22,41 @@ def new end def create - project_ids = params[:milestone][:project_ids].reject(&:blank?) - title = milestone_params[:title] + @milestone = Milestones::CreateService.new(group, current_user, milestone_params).execute - if create_milestones(project_ids) - redirect_to milestone_path(title) + if @milestone.persisted? + redirect_to milestone_path else - render_new_with_error(project_ids.empty?) + render "new" end end def show end - def update - @milestone.milestones.each do |milestone| - Milestones::UpdateService.new(milestone.project, current_user, milestone_params).execute(milestone) - end - - redirect_back_or_default(default: milestone_path(@milestone.title)) + def edit + render_404 if @milestone.is_legacy_group_milestone? end - private - - def create_milestones(project_ids) - return false unless project_ids.present? + def update + # Keep this compatible with legacy group milestones where we have to update + # all projects milestones states at once. + if @milestone.is_legacy_group_milestone? + update_params = milestone_params.select { |key| key == "state_event" } + milestones = @milestone.milestones + else + update_params = milestone_params + milestones = [@milestone] + end - ActiveRecord::Base.transaction do - @projects.where(id: project_ids).each do |project| - Milestones::CreateService.new(project, current_user, milestone_params).execute - end + milestones.each do |milestone| + Milestones::UpdateService.new(milestone.parent, current_user, update_params).execute(milestone) end - true - rescue ActiveRecord::ActiveRecordError => e - flash.now[:alert] = "An error occurred while creating the milestone: #{e.message}" - false + redirect_to milestone_path end - def render_new_with_error(empty_project_ids) - @milestone = Milestone.new(milestone_params) - @milestone.errors.add(:base, "Please select at least one project.") if empty_project_ids - render :new - end + private def authorize_admin_milestones! return render_404 unless can?(current_user, :admin_milestones, group) @@ -74,16 +66,31 @@ def milestone_params params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event) end - def milestone_path(title) - group_milestone_path(@group, title.to_slug.to_s, title: title) + def milestone_path + if @milestone.is_legacy_group_milestone? + group_milestone_path(group, @milestone.safe_title, title: @milestone.title) + else + group_milestone_path(group, @milestone.iid) + end end def milestones - @milestones = GroupMilestone.build_collection(@group, @projects, params) + search_params = params.merge(group_ids: group.id) + + milestones = MilestonesFinder.new(search_params).execute + legacy_milestones = GroupMilestone.build_collection(group, group_projects, params) + + milestones + legacy_milestones end def milestone - @milestone = GroupMilestone.build(@group, @projects, params[:title]) + @milestone = + if params[:title] + GroupMilestone.build(group, group_projects, params[:title]) + else + group.milestones.find_by_iid(params[:id]) + end + render_404 unless @milestone end end diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb index d1c0be5a8eea521dad054fce0ef0f0fd032a5691..3c9a7dac9d4d4e7be5440b0caff1fe414c9b7f24 100644 --- a/app/controllers/projects/milestones_controller.rb +++ b/app/controllers/projects/milestones_controller.rb @@ -13,20 +13,16 @@ class Projects::MilestonesController < Projects::ApplicationController respond_to :html def index - @milestones = - case params[:state] - when 'all' then @project.milestones - when 'closed' then @project.milestones.closed - else @project.milestones.active - end - @sort = params[:sort] || 'due_date_asc' - @milestones = @milestones.sort(@sort) + @milestones = milestones.sort(@sort) respond_to do |format| format.html do @project_namespace = @project.namespace.becomes(Namespace) - @milestones = @milestones.includes(:project) + # We need to show group milestones in the JSON response + # so that people can filter by and assign group milestones, + # but we don't need to show them on the project milestones page itself. + @milestones = @milestones.for_projects @milestones = @milestones.page(params[:page]) end format.json do @@ -54,7 +50,7 @@ def show def create @milestone = Milestones::CreateService.new(project, current_user, milestone_params).execute - if @milestone.save + if @milestone.valid? redirect_to project_milestone_path(@project, @milestone) else render "new" @@ -89,6 +85,18 @@ def destroy protected + def milestones + @milestones ||= begin + if @project.group && can?(current_user, :read_group, @project.group) + group = @project.group + end + + search_params = params.merge(project_ids: @project.id, group_ids: group&.id) + + MilestonesFinder.new(search_params).execute + end + end + def milestone @milestone ||= @project.milestones.find_by!(iid: params[:id]) end diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 83b43d4daa7dc042556ee53fda8fd70f2304403a..9f54525a55ce68f1c0602fede928fc3c006ecb05 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -154,9 +154,17 @@ def milestones @milestones = if milestones? - scope = Milestone.where(project_id: projects) + if project? + group_id = project.group&.id + project_id = project.id + end + + group_id = group.id if group - scope.where(title: params[:milestone_title]) + search_params = + { title: params[:milestone_title], project_ids: project_id, group_ids: group_id } + + MilestonesFinder.new(search_params).execute else Milestone.none end @@ -338,11 +346,6 @@ def by_milestone(items) items = items.left_joins_milestones.where('milestones.start_date <= NOW()') else items = items.with_milestone(params[:milestone_title]) - items_projects = projects(items) - - if items_projects - items = items.where(milestones: { project_id: items_projects }) - end end end diff --git a/app/finders/milestones_finder.rb b/app/finders/milestones_finder.rb index 630c73c2a94bd79f63e89a861614b9fe3a26ad3b..0a5a0ea2f35e373e486a64e9b2cd4f9353c01d02 100644 --- a/app/finders/milestones_finder.rb +++ b/app/finders/milestones_finder.rb @@ -1,12 +1,56 @@ +# Search for milestones +# +# params - Hash +# project_ids: Array of project ids or single project id. +# group_ids: Array of group ids or single group id. +# order - Orders by field default due date asc. +# title - filter by title. +# state - filters by state. + class MilestonesFinder - def execute(projects, params) - milestones = Milestone.of_projects(projects) - milestones = milestones.reorder("due_date ASC") - - case params[:state] - when 'closed' then milestones.closed - when 'all' then milestones - else milestones.active + attr_reader :params, :project_ids, :group_ids + + def initialize(params = {}) + @project_ids = Array(params[:project_ids]) + @group_ids = Array(params[:group_ids]) + @params = params + end + + def execute + return Milestone.none if project_ids.empty? && group_ids.empty? + + items = Milestone.all + items = by_groups_and_projects(items) + items = by_title(items) + items = by_state(items) + + order(items) + end + + private + + def by_groups_and_projects(items) + items.for_projects_and_groups(project_ids, group_ids) + end + + def by_title(items) + if params[:title] + items.where(title: params[:title]) + else + items + end + end + + def by_state(items) + Milestone.filter_by_state(items, params[:state]) + end + + def order(items) + if params.has_key?(:order) + items.reorder(params[:order]) + else + order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC') + items.reorder(order_statement) end end end diff --git a/app/helpers/milestones_helper.rb b/app/helpers/milestones_helper.rb index be0277cf6b529dba4ba563db35098e75abe7fa19..dbc4e211a666b809cf91caf13bb34ad5e77d25b5 100644 --- a/app/helpers/milestones_helper.rb +++ b/app/helpers/milestones_helper.rb @@ -54,8 +54,10 @@ def milestone_counts(milestones) def milestone_class_for_state(param, check, match_blank_param = false) if match_blank_param 'active' if param.blank? || param == check + elsif param == check + 'active' else - 'active' if param == check + check end end @@ -179,4 +181,14 @@ def milestone_labels_tab_path(milestone) labels_dashboard_milestone_path(milestone, title: milestone.title, format: :json) end end + + def group_milestone_route(milestone, params = {}) + params = nil if params.empty? + + if milestone.is_legacy_group_milestone? + group_milestone_path(@group, milestone.safe_title, title: milestone.title, milestone: params) + else + group_milestone_path(@group, milestone.iid, milestone: params) + end + end end diff --git a/app/models/concerns/internal_id.rb b/app/models/concerns/internal_id.rb index 5382dde67654b528847a58dde8cd806d593d0a49..67a0adfcd563b8d77f9c67e895eb2abd7d3e7b2d 100644 --- a/app/models/concerns/internal_id.rb +++ b/app/models/concerns/internal_id.rb @@ -8,7 +8,8 @@ module InternalId def set_iid if iid.blank? - records = project.send(self.class.name.tableize) + parent = project || group + records = parent.send(self.class.name.tableize) records = records.with_deleted if self.paranoid? max_iid = records.maximum(:iid) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 7c1957e01135f9e316370025910ce1bcd9f3bb91..8e34b0c04b7b614613ed09cf7c5379874bd18d01 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -30,6 +30,7 @@ module Issuable belongs_to :updated_by, class_name: "User" belongs_to :last_edited_by, class_name: 'User' belongs_to :milestone + has_many :notes, as: :noteable, inverse_of: :noteable, dependent: :destroy do # rubocop:disable Cop/ActiveRecordDependent def authors_loaded? # We check first if we're loaded to not load unnecessarily. diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb index 01599ce49c64250116997aa5ff9c3c50b779f092..f09984658222fb968166d4f91904bdf7db88cd9a 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -70,6 +70,22 @@ def expired? due_date && due_date.past? end + def is_group_milestone? + false + end + + def is_project_milestone? + false + end + + def is_legacy_group_milestone? + false + end + + def is_dashboard_milestone? + false + end + private def count_issues_by_state(user) diff --git a/app/models/dashboard_milestone.rb b/app/models/dashboard_milestone.rb index 646c1e5ce1a364a5c17138f4384d37dd1e02961d..fac7c5e5c85eafcbe6e9f7d21f23adc86ff5d855 100644 --- a/app/models/dashboard_milestone.rb +++ b/app/models/dashboard_milestone.rb @@ -2,4 +2,8 @@ class DashboardMilestone < GlobalMilestone def issues_finder_params { authorized_only: true } end + + def is_dashboard_milestone? + true + end end diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb index 6b5dd9f3c5391839943052f82d8f26924e6099dc..218743d2eb62906ccb744af6873016162c420b33 100644 --- a/app/models/global_milestone.rb +++ b/app/models/global_milestone.rb @@ -4,6 +4,7 @@ class GlobalMilestone include ::EE::GlobalMilestone EPOCH = DateTime.parse('1970-01-01') + STATE_COUNT_HASH = { opened: 0, closed: 0, all: 0 }.freeze attr_accessor :title, :milestones alias_attribute :name, :title @@ -13,7 +14,10 @@ def for_display end def self.build_collection(projects, params) - child_milestones = MilestonesFinder.new.execute(projects, params) + params = + { project_ids: projects.map(&:id), state: params[:state] } + + child_milestones = MilestonesFinder.new(params).execute milestones = child_milestones.select(:id, :title).group_by(&:title).map do |title, grouped| milestones_relation = Milestone.where(id: grouped.map(&:id)) @@ -30,13 +34,42 @@ def self.build(projects, title) new(title, child_milestones) end - def self.states_count(projects) - relation = MilestonesFinder.new.execute(projects, state: 'all') - milestones_by_state_and_title = relation.reorder(nil).group(:state, :title).count + def self.states_count(projects, group = nil) + legacy_group_milestones_count = legacy_group_milestone_states_count(projects) + group_milestones_count = group_milestones_states_count(group) + + legacy_group_milestones_count.merge(group_milestones_count) do |k, legacy_group_milestones_count, group_milestones_count| + legacy_group_milestones_count + group_milestones_count + end + end + + def self.group_milestones_states_count(group) + return STATE_COUNT_HASH unless group + + params = { group_ids: [group.id], state: 'all', order: nil } + + relation = MilestonesFinder.new(params).execute + grouped_by_state = relation.group(:state).count + + { + opened: grouped_by_state['active'] || 0, + closed: grouped_by_state['closed'] || 0, + all: grouped_by_state.values.sum + } + end + + # Counts the legacy group milestones which must be grouped by title + def self.legacy_group_milestone_states_count(projects) + return STATE_COUNT_HASH unless projects + + params = { project_ids: projects.map(&:id), state: 'all', order: nil } + + relation = MilestonesFinder.new(params).execute + project_milestones_by_state_and_title = relation.group(:state, :title).count - opened = count_by_state(milestones_by_state_and_title, 'active') - closed = count_by_state(milestones_by_state_and_title, 'closed') - all = milestones_by_state_and_title.map { |(_, title), _| title }.uniq.count + opened = count_by_state(project_milestones_by_state_and_title, 'active') + closed = count_by_state(project_milestones_by_state_and_title, 'closed') + all = project_milestones_by_state_and_title.map { |(_, title), _| title }.uniq.count { opened: opened, diff --git a/app/models/group.rb b/app/models/group.rb index 9e4280c52977541811ce66c196b66a3241b93e93..6905b117ac473d0ee18388a22c2a82d5a1ffeb80 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -22,6 +22,7 @@ class Group < Namespace has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent + has_many :milestones has_many :project_group_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :shared_projects, through: :project_group_links, source: :project has_many :notification_settings, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/models/group_milestone.rb b/app/models/group_milestone.rb index de71fe87c59dc3b279fdd8860629cda551a74d25..83d6f79039ece5eecac19e640d35fc6196f01379 100644 --- a/app/models/group_milestone.rb +++ b/app/models/group_milestone.rb @@ -18,4 +18,8 @@ def self.build(group, projects, title) def issues_finder_params { group_id: group.id } end + + def is_legacy_group_milestone? + true + end end diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 40b80a91b1fc3135ddb416346ab196b86eb099a8..3e7262b98613edac564da8df72e777c72fe1f1f9 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -21,18 +21,33 @@ class Milestone < ActiveRecord::Base cache_markdown_field :description belongs_to :project + belongs_to :group + has_many :boards has_many :issues has_many :labels, -> { distinct.reorder('labels.title') }, through: :issues has_many :merge_requests has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + scope :of_projects, ->(ids) { where(project_id: ids) } + scope :of_groups, ->(ids) { where(group_id: ids) } scope :active, -> { with_state(:active) } scope :closed, -> { with_state(:closed) } - scope :of_projects, ->(ids) { where(project_id: ids) } + scope :for_projects, -> { where(group: nil).includes(:project) } + + scope :for_projects_and_groups, -> (project_ids, group_ids) do + conditions = [] + conditions << arel_table[:project_id].in(project_ids) if project_ids.compact.any? + conditions << arel_table[:group_id].in(group_ids) if group_ids.compact.any? + + where(conditions.reduce(:or)) + end + + validates :group, presence: true, unless: :project + validates :project, presence: true, unless: :group - validates :title, presence: true, uniqueness: { scope: :project_id } - validates :project, presence: true + validate :uniqueness_of_title, if: :title_changed? + validate :milestone_type_check validate :start_date_should_be_less_than_due_date, if: proc { |m| m.start_date.present? && m.due_date.present? } strip_attributes :title @@ -67,6 +82,14 @@ def search(query) where(t[:title].matches(pattern).or(t[:description].matches(pattern))) end + + def filter_by_state(milestones, state) + case state + when 'closed' then milestones.closed + when 'all' then milestones + else milestones.active + end + end end def self.reference_prefix @@ -142,6 +165,8 @@ def self.sort(method) # Milestone.first.to_reference(same_namespace_project) # => "gitlab-ce%1" # def to_reference(from_project = nil, format: :iid, full: false) + return if is_group_milestone? + format_reference = milestone_format_reference(format) reference = "#{self.class.reference_prefix}#{format_reference}" @@ -156,6 +181,10 @@ def milestoneish_ids id end + def for_display + self + end + def can_be_closed? active? && issues.opened.count.zero? end @@ -168,8 +197,45 @@ def title=(value) write_attribute(:title, sanitize_title(value)) if value.present? end + def safe_title + title.to_slug.normalize.to_s + end + + def parent + group || project + end + + def is_group_milestone? + group_id.present? + end + + def is_project_milestone? + project_id.present? + end + private + # Milestone titles must be unique across project milestones and group milestones + def uniqueness_of_title + if project + relation = Milestone.for_projects_and_groups([project_id], [project.group&.id]) + elsif group + project_ids = group.projects.map(&:id) + relation = Milestone.for_projects_and_groups(project_ids, [group.id]) + end + + title_exists = relation.find_by_title(title) + errors.add(:title, "already being used for another group or project milestone.") if title_exists + end + + # Milestone should be either a project milestone or a group milestone + def milestone_type_check + if group_id && project_id + field = project_id_changed? ? :project_id : :group_id + errors.add(field, "milestone should belong either to a project or a group.") + end + end + def milestone_format_reference(format = :iid) raise ArgumentError, 'Unknown format' unless [:iid, :name].include?(format) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 25ee69e5ebcbde4d93407725969d8b9dc435eba9..6bd6f84fce06e95be05a90b1ca405aab6ec09099 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -4,8 +4,11 @@ class IssuableBaseService < BaseService private def create_milestone_note(issuable) + milestone = issuable.milestone + return if milestone && milestone.is_group_milestone? + SystemNoteService.change_milestone( - issuable, issuable.project, current_user, issuable.milestone) + issuable, issuable.project, current_user, milestone) end def create_labels_note(issuable, old_labels) @@ -91,10 +94,12 @@ def filter_milestone milestone_id = params[:milestone_id] return unless milestone_id - if milestone_id == IssuableFinder::NONE || - project.milestones.find_by(id: milestone_id).nil? - params[:milestone_id] = '' - end + params[:milestone_id] = '' if milestone_id == IssuableFinder::NONE + + milestone = + Milestone.for_projects_and_groups([project.id], [project.group&.id]).find_by_id(milestone_id) + + params[:milestone_id] = '' unless milestone end def filter_labels diff --git a/app/services/issues/move_service.rb b/app/services/issues/move_service.rb index 711f4035c55a4ae6617c1b1d0a102c7c624e61de..29def25719d0a02093a8c17eb2088d7ffa4ba390 100644 --- a/app/services/issues/move_service.rb +++ b/app/services/issues/move_service.rb @@ -61,8 +61,18 @@ def cloneable_label_ids end def cloneable_milestone_id - @new_project.milestones - .find_by(title: @old_issue.milestone.try(:title)).try(:id) + title = @old_issue.milestone&.title + return unless title + + if @new_project.group && can?(current_user, :read_group, @new_project.group) + group_id = @new_project.group.id + end + + params = + { title: title, project_ids: @new_project.id, group_ids: group_id } + + milestones = MilestonesFinder.new(params).execute + milestones.first&.id end def rewrite_notes diff --git a/app/services/milestones/base_service.rb b/app/services/milestones/base_service.rb index 176ab9f1ab5ca909bb6e2cd424a7e61586f63146..4963601ea8b9bf7ef2438f13c0c27332a6ce84c8 100644 --- a/app/services/milestones/base_service.rb +++ b/app/services/milestones/base_service.rb @@ -1,4 +1,10 @@ module Milestones class BaseService < ::BaseService + # Parent can either a group or a project + attr_accessor :parent, :current_user, :params + + def initialize(parent, user, params = {}) + @parent, @current_user, @params = parent, user, params.dup + end end end diff --git a/app/services/milestones/close_service.rb b/app/services/milestones/close_service.rb index 608fc49d7669b31493f2f24cd60eb2c0eb0cb074..776ec4b287b1df5f9d2346c97c108ea681b77858 100644 --- a/app/services/milestones/close_service.rb +++ b/app/services/milestones/close_service.rb @@ -1,7 +1,7 @@ module Milestones class CloseService < Milestones::BaseService def execute(milestone) - if milestone.close + if milestone.close && milestone.is_project_milestone? event_service.close_milestone(milestone, current_user) end diff --git a/app/services/milestones/create_service.rb b/app/services/milestones/create_service.rb index b8e08c9f1eb167e97f496f359819b5c013c1aeb5..aef3124c7e3f28e41c707edbd7a7cc620048b7b0 100644 --- a/app/services/milestones/create_service.rb +++ b/app/services/milestones/create_service.rb @@ -1,9 +1,9 @@ module Milestones class CreateService < Milestones::BaseService def execute - milestone = project.milestones.new(params) + milestone = parent.milestones.new(params) - if milestone.save + if milestone.save && milestone.is_project_milestone? event_service.open_milestone(milestone, current_user) end diff --git a/app/services/milestones/reopen_service.rb b/app/services/milestones/reopen_service.rb index 573f9ee5c2100c6e4318f904215c96f1c5a4338e..5b8b682caafd7a3f085c85f170e897b26d7be12a 100644 --- a/app/services/milestones/reopen_service.rb +++ b/app/services/milestones/reopen_service.rb @@ -1,7 +1,7 @@ module Milestones class ReopenService < Milestones::BaseService def execute(milestone) - if milestone.activate + if milestone.activate && milestone.is_project_milestone? event_service.reopen_milestone(milestone, current_user) end diff --git a/app/services/milestones/update_service.rb b/app/services/milestones/update_service.rb index ed64847f429590bec4962afd3343054726ec3d27..31b441ed476133aff3962d5e0bdeb43fd078faa7 100644 --- a/app/services/milestones/update_service.rb +++ b/app/services/milestones/update_service.rb @@ -5,9 +5,9 @@ def execute(milestone) case state when 'activate' - Milestones::ReopenService.new(project, current_user, {}).execute(milestone) + Milestones::ReopenService.new(parent, current_user, {}).execute(milestone) when 'close' - Milestones::CloseService.new(project, current_user, {}).execute(milestone) + Milestones::CloseService.new(parent, current_user, {}).execute(milestone) end if params.present? diff --git a/app/views/groups/milestones/_form.html.haml b/app/views/groups/milestones/_form.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..7f450cd9a930f45a10187885ea1595395ee654d3 --- /dev/null +++ b/app/views/groups/milestones/_form.html.haml @@ -0,0 +1,27 @@ += form_for [@group, @milestone], html: { class: 'form-horizontal milestone-form common-note-form js-quick-submit js-requires-input' } do |f| + .row + = form_errors(@milestone) + + .col-md-6 + .form-group + = f.label :title, "Title", class: "control-label" + .col-sm-10 + = f.text_field :title, maxlength: 255, class: "form-control", required: true, autofocus: true + .form-group.milestone-description + = f.label :description, "Description", class: "control-label" + .col-sm-10 + = render layout: 'projects/md_preview', locals: { url: '' } do + = render 'projects/zen', f: f, attr: :description, classes: 'note-textarea', placeholder: 'Write milestone description...' + .clearfix + .error-alert + + = render "shared/milestones/form_dates", f: f + + .form-actions + - if @milestone.new_record? + = f.submit 'Create milestone', class: "btn-create btn" + = link_to "Cancel", group_milestones_path(@group), class: "btn btn-cancel" + - else + = f.submit 'Update milestone', class: "btn-create btn" + = link_to "Cancel", group_milestone_path(@group, @milestone), class: "btn btn-cancel" + diff --git a/app/views/groups/milestones/_milestone.html.haml b/app/views/groups/milestones/_milestone.html.haml index 4c4e0a267288dc0459721c9aeae0dcfb5b4ae9b7..bae8997e24c093d5aba8fc08851a2dc336bb0ee0 100644 --- a/app/views/groups/milestones/_milestone.html.haml +++ b/app/views/groups/milestones/_milestone.html.haml @@ -1,5 +1,6 @@ + = render 'shared/milestones/milestone', - milestone_path: group_milestone_path(@group, milestone.safe_title, title: milestone.title), + milestone_path: group_milestone_route(milestone), issues_path: issues_group_path(@group, milestone_title: milestone.title), merge_requests_path: merge_requests_group_path(@group, milestone_title: milestone.title), milestone: milestone diff --git a/app/views/groups/milestones/edit.html.haml b/app/views/groups/milestones/edit.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..5f6d7d209d0627b01faa270c856573d77cecdb43 --- /dev/null +++ b/app/views/groups/milestones/edit.html.haml @@ -0,0 +1,7 @@ +- page_title "Milestones" +- render "header_title" + +%h3.page-title + Edit Milestone + += render "form" diff --git a/app/views/groups/milestones/index.html.haml b/app/views/groups/milestones/index.html.haml index f91bee0b6109a549b708ff6ddd4f042675e79490..6ceb409230712f9e7490cf2e918f6f41edb3b213 100644 --- a/app/views/groups/milestones/index.html.haml +++ b/app/views/groups/milestones/index.html.haml @@ -9,11 +9,6 @@ = link_to new_group_milestone_path(@group), class: "btn btn-new" do New milestone -.row-content-block - Only milestones from - %strong= @group.name - group are listed here. - .milestones %ul.content-list - if @milestones.blank? diff --git a/app/views/groups/milestones/new.html.haml b/app/views/groups/milestones/new.html.haml index 7c7573862d02ce1c26edea11378a7bc840a402ea..e24844661ee70bb0edad020b27cd2ff7873684c2 100644 --- a/app/views/groups/milestones/new.html.haml +++ b/app/views/groups/milestones/new.html.haml @@ -4,40 +4,4 @@ %h3.page-title New Milestone -%p.light - This will create milestone in every selected project -%hr - -= form_for @milestone, url: group_milestones_path(@group), html: { class: 'form-horizontal milestone-form common-note-form js-quick-submit js-requires-input' } do |f| - .row - - if @milestone.errors.any? - #error_explanation - .alert.alert-danger - %ul - - @milestone.errors.full_messages.each do |msg| - %li - = msg - - .col-md-6 - .form-group - = f.label :title, "Title", class: "control-label" - .col-sm-10 - = f.text_field :title, maxlength: 255, class: "form-control", required: true, autofocus: true - .form-group.milestone-description - = f.label :description, "Description", class: "control-label" - .col-sm-10 - = render layout: 'projects/md_preview', locals: { url: '' } do - = render 'projects/zen', f: f, attr: :description, classes: 'note-textarea', placeholder: 'Write milestone description...' - .clearfix - .error-alert - .form-group - = f.label :projects, "Projects", class: "control-label" - .col-sm-10 - = f.collection_select :project_ids, @group.projects.non_archived, :id, :name, - { selected: @group.projects.non_archived.pluck(:id) }, required: true, multiple: true, class: 'select2' - - = render "shared/milestones/form_dates", f: f - - .form-actions - = f.submit 'Create milestone', class: "btn-create btn" - = link_to "Cancel", group_milestones_path(@group), class: "btn btn-cancel" += render "form" diff --git a/app/views/groups/milestones/show.html.haml b/app/views/groups/milestones/show.html.haml index 33e68bc766edc4c7fc181f20f1a6f8efd47bba53..54b1b7a734a55258e3a6e6dcc637035bff88b351 100644 --- a/app/views/groups/milestones/show.html.haml +++ b/app/views/groups/milestones/show.html.haml @@ -1,4 +1,4 @@ = render "header_title" = render 'shared/milestones/top', milestone: @milestone, group: @group -= render 'shared/milestones/tabs', milestone: @milestone, show_project_name: true += render 'shared/milestones/tabs', milestone: @milestone, show_project_name: true if @milestone.is_legacy_group_milestone? = render 'shared/milestones/sidebar', milestone: @milestone, affix_offset: 102 diff --git a/app/views/shared/milestones/_milestone.html.haml b/app/views/shared/milestones/_milestone.html.haml index ecc8b42979cbccfec756867bfd16a7b15a151bfb..6f6a036b13fe3eb6a82ad28fb0edf6905d0af721 100644 --- a/app/views/shared/milestones/_milestone.html.haml +++ b/app/views/shared/milestones/_milestone.html.haml @@ -1,10 +1,15 @@ - dashboard = local_assigns[:dashboard] -- custom_dom_id = dom_id(@project ? milestone : milestone.milestones.first) +- custom_dom_id = dom_id(milestone.try(:milestones) ? milestone.milestones.first : milestone) %li{ class: "milestone milestone-#{milestone.closed? ? 'closed' : 'open'}", id: custom_dom_id } .row .col-sm-6 %strong= link_to truncate(milestone.title, length: 100), milestone_path + - if milestone.is_group_milestone? + %span - Group Milestone + - else + %span - Project Milestone + .col-sm-6 .pull-right.light #{milestone.percent_complete(current_user)}% complete .row @@ -13,26 +18,32 @@ · = link_to pluralize(milestone.merge_requests.size, 'Merge Request'), merge_requests_path .col-sm-6= milestone_progress_bar(milestone) - - if milestone.is_a?(GlobalMilestone) + - if milestone.is_a?(GlobalMilestone) || milestone.is_group_milestone? .row .col-sm-6 - .expiration= render('shared/milestone_expired', milestone: milestone) - .projects - - milestone.milestones.each do |milestone| - = link_to milestone_path(milestone) do - %span.label.label-gray - = dashboard ? milestone.project.name_with_namespace : milestone.project.name + - if milestone.is_legacy_group_milestone? + .expiration= render('shared/milestone_expired', milestone: milestone) + .projects + - milestone.milestones.each do |milestone| + = link_to milestone_path(milestone) do + %span.label.label-gray + = dashboard ? milestone.project.name_with_namespace : milestone.project.name - if @group - .col-sm-6 + .col-sm-6.milestone-actions - if can?(current_user, :admin_milestones, @group) + - if milestone.is_group_milestone? + = link_to edit_group_milestone_path(@group, milestone.id), class: "btn btn-xs btn-grouped" do + Edit + \ - if milestone.closed? - = link_to 'Reopen Milestone', group_milestone_path(@group, milestone.safe_title, title: milestone.title, milestone: {state_event: :activate }), method: :put, class: "btn btn-xs btn-grouped btn-reopen" + = link_to 'Reopen Milestone', group_milestone_route(milestone, {state_event: :activate }), method: :put, class: "btn btn-xs btn-grouped btn-reopen" - else - = link_to 'Close Milestone', group_milestone_path(@group, milestone.safe_title, title: milestone.title, milestone: {state_event: :close }), method: :put, class: "btn btn-xs btn-close" + = link_to 'Close Milestone', group_milestone_route(milestone, {state_event: :close }), method: :put, class: "btn btn-xs btn-grouped btn-close" - if @project .row - .col-sm-6= render('shared/milestone_expired', milestone: milestone) + .col-sm-6 + = render('shared/milestone_expired', milestone: milestone) .col-sm-6.milestone-actions - if can?(current_user, :admin_milestone, milestone.project) and milestone.active? = link_to edit_project_milestone_path(milestone.project, milestone), class: "btn btn-xs btn-grouped" do diff --git a/app/views/shared/milestones/_top.html.haml b/app/views/shared/milestones/_top.html.haml index 20a12613cfc1ba6ba0930b2f75a446feeb599b28..b93837e3087a121625f013ada3c713a4a958f173 100644 --- a/app/views/shared/milestones/_top.html.haml +++ b/app/views/shared/milestones/_top.html.haml @@ -22,39 +22,55 @@ - if group .pull-right - if can?(current_user, :admin_milestones, group) + - if milestone.is_group_milestone? + = link_to edit_group_milestone_path(group, milestone.iid), class: "btn btn btn-grouped" do + Edit - if milestone.active? - = link_to 'Close Milestone', group_milestone_path(group, milestone.safe_title, title: milestone.title, milestone: {state_event: :close }), method: :put, class: "btn btn-grouped btn-close" + = link_to 'Close Milestone', group_milestone_route(milestone, {state_event: :close }), method: :put, class: "btn btn-grouped btn-close" - else - = link_to 'Reopen Milestone', group_milestone_path(group, milestone.safe_title, title: milestone.title, milestone: {state_event: :activate }), method: :put, class: "btn btn-grouped btn-reopen" + = link_to 'Reopen Milestone', group_milestone_route(milestone, {state_event: :activate }), method: :put, class: "btn btn-grouped btn-reopen" .detail-page-description.milestone-detail %h2.title = markdown_field(milestone, :title) + - if @milestone.is_group_milestone? && @milestone.description.present? + %div + .description + .wiki + = markdown_field(@milestone, :description) - if milestone.complete?(current_user) && milestone.active? .alert.alert-success.prepend-top-default - close_msg = group ? 'You may close the milestone now.' : 'Navigate to the project to close the milestone.' %span All issues for this milestone are closed. #{close_msg} -.table-holder - %table.table - %thead - %tr - %th Project - %th Open issues - %th State - %th Due date - - milestone.milestones.each do |ms| - %tr - %td - - project_name = group ? ms.project.name : ms.project.name_with_namespace - = link_to project_name, project_milestone_path(ms.project, ms) - %td - = ms.issues_visible_to_user(current_user).opened.count - %td - - if ms.closed? - Closed - - else - Open - %td - = ms.expires_at +- if @milestone.is_legacy_group_milestone? || @milestone.is_dashboard_milestone? + .table-holder + %table.table + %thead + %tr + %th Project + %th Open issues + %th State + %th Due date + - milestone.milestones.each do |ms| + %tr + %td + - project_name = group ? ms.project.name : ms.project.name_with_namespace + = link_to project_name, project_milestone_path(ms.project, ms) + %td + = ms.issues_visible_to_user(current_user).opened.count + %td + - if ms.closed? + Closed + - else + Open + %td + = ms.expires_at +- elsif @milestone.is_group_milestone? + %br + View + = link_to 'Issues', issues_group_path(@group, milestone_title: milestone.title) + or + = link_to 'Merge Requests', merge_requests_group_path(@group, milestone_title: milestone.title) + in this milestone diff --git a/changelogs/unreleased/issue_30126_be.yml b/changelogs/unreleased/issue_30126_be.yml new file mode 100644 index 0000000000000000000000000000000000000000..96bb8d9574ba29e8010614ca92e6e094ad24a72c --- /dev/null +++ b/changelogs/unreleased/issue_30126_be.yml @@ -0,0 +1,4 @@ +--- +title: Add native group milestones +merge_request: +author: diff --git a/config/routes/group.rb b/config/routes/group.rb index 9e84b9dd122cd06f85f1aa569a6fb403ef9447fa..859a47488d22875743a6832bbf2537747b49b351 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -28,7 +28,7 @@ end resource :avatar, only: [:destroy] - resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :update, :new, :create] do + resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :edit, :update, :new, :create] do member do get :merge_requests get :participants diff --git a/db/migrate/20170707183807_add_group_id_to_milestones.rb b/db/migrate/20170707183807_add_group_id_to_milestones.rb new file mode 100644 index 0000000000000000000000000000000000000000..675ffd4a1c92f1fbece23dc3c1053179044bfc43 --- /dev/null +++ b/db/migrate/20170707183807_add_group_id_to_milestones.rb @@ -0,0 +1,20 @@ +class AddGroupIdToMilestones < ActiveRecord::Migration + DOWNTIME = false + + def up + return if column_exists? :milestones, :group_id + + change_column_null :milestones, :project_id, true + + add_column :milestones, :group_id, :integer + end + + def down + # We cannot rollback project_id not null constraint if there are records + # with null values. + execute "DELETE from milestones WHERE project_id IS NULL" + + remove_column :milestones, :group_id + change_column :milestones, :project_id, :integer, null: false + end +end diff --git a/db/migrate/20170707184243_add_group_milestone_id_indexes.rb b/db/migrate/20170707184243_add_group_milestone_id_indexes.rb new file mode 100644 index 0000000000000000000000000000000000000000..aa48fe90cadb6d899e9b4efb21b82312cc4d6eac --- /dev/null +++ b/db/migrate/20170707184243_add_group_milestone_id_indexes.rb @@ -0,0 +1,21 @@ +class AddGroupMilestoneIdIndexes < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + DOWNTIME = false + + def up + return if index_exists?(:milestones, :group_id) + + add_concurrent_foreign_key :milestones, :namespaces, column: :group_id, on_delete: :cascade + + add_concurrent_index :milestones, :group_id + end + + def down + remove_foreign_key :milestones, column: :group_id + + remove_concurrent_index :milestones, :group_id + end +end diff --git a/db/migrate/20170707184244_remove_wrong_versions_from_schema_versions.rb b/db/migrate/20170707184244_remove_wrong_versions_from_schema_versions.rb new file mode 100644 index 0000000000000000000000000000000000000000..38536a8b06a23224d1f18f5f2357db193a1d895d --- /dev/null +++ b/db/migrate/20170707184244_remove_wrong_versions_from_schema_versions.rb @@ -0,0 +1,10 @@ +class RemoveWrongVersionsFromSchemaVersions < ActiveRecord::Migration + DOWNTIME = false + + def up + execute("DELETE FROM schema_migrations WHERE version IN ('20170723183807', '20170724184243')") + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index 9b32f43d42e92ba94dcf13b5ad4a54e9bf9b8282..873bdc95c1436d96978e651596fdb30b0326dc2c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170706151212) do +ActiveRecord::Schema.define(version: 20170707184244) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1010,7 +1010,7 @@ create_table "milestones", force: :cascade do |t| t.string "title", null: false - t.integer "project_id", null: false + t.integer "project_id" t.text "description" t.date "due_date" t.datetime "created_at" @@ -1021,10 +1021,12 @@ t.text "description_html" t.date "start_date" t.integer "cached_markdown_version" + t.integer "group_id" end add_index "milestones", ["description"], name: "index_milestones_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"} add_index "milestones", ["due_date"], name: "index_milestones_on_due_date", using: :btree + add_index "milestones", ["group_id"], name: "index_milestones_on_group_id", using: :btree add_index "milestones", ["project_id", "iid"], name: "index_milestones_on_project_id_and_iid", unique: true, using: :btree add_index "milestones", ["title"], name: "index_milestones_on_title", using: :btree add_index "milestones", ["title"], name: "index_milestones_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"} @@ -1920,6 +1922,7 @@ add_foreign_key "merge_requests", "projects", column: "target_project_id", name: "fk_a6963e8447", on_delete: :cascade add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade add_foreign_key "merge_requests_closing_issues", "merge_requests", on_delete: :cascade + add_foreign_key "milestones", "namespaces", column: "group_id", name: "fk_95650a40d4", on_delete: :cascade add_foreign_key "milestones", "projects", name: "fk_9bd0a0c791", on_delete: :cascade add_foreign_key "namespace_statistics", "namespaces", on_delete: :cascade add_foreign_key "notes", "projects", name: "fk_99e097b079", on_delete: :cascade diff --git a/doc/user/project/milestones/index.md b/doc/user/project/milestones/index.md index 36ef52546e817f0f47fa3c75b2bcad5270d10fd8..eaec15ed63f828b3252396f50cf47a3bf09a88ae 100644 --- a/doc/user/project/milestones/index.md +++ b/doc/user/project/milestones/index.md @@ -51,14 +51,11 @@ The start/due dates are required if you intend to use [Burndown charts](#burndow >**Note:** You need [Master permissions](../../permissions.md) in order to create a milestone. -You can create a milestone for several projects in the same group simultaneously. -On the group's **Issues ➔ Milestones** page, you will be able to see the status -of that milestone across all of the selected projects. To create a new milestone -for selected projects in the group, click the **New milestone** button. The -form is the same as when creating a milestone for a specific project with the -addition of the selection of the projects you want to inherit this milestone. - -![Creating a group milestone](img/milestone_group_create.png) +You can create a milestone for a group that will be shared across group projects. +On the group's **Issues ➔ Milestones** page, you will be able to see the state +of that milestone and the issues/merge requests count that it shares across the group projects. To create a new milestone click the **New milestone** button. The form is the same as when creating a milestone for a specific project which you can find in the previous item. + +In addition to that you will be able to filter issues or merge requests by group milestones in all projects that belongs to the milestone group. ## Special milestone filters diff --git a/features/group/milestones.feature b/features/group/milestones.feature index 1c1539b3e126247d3c5fc44fcaf21473b52e3a7b..2211acfee20fa3c25f31a94b9c2225d8ea08fa7d 100644 --- a/features/group/milestones.feature +++ b/features/group/milestones.feature @@ -22,12 +22,12 @@ Feature: Group Milestones Then I should see group milestone with descriptions and expiry date And I should see group milestone with all issues and MRs assigned to that milestone - Scenario: Create multiple milestones with one form + Scenario: Create group milestones Given I visit group "Owned" milestones page And I click new milestone button And I fill milestone name When I press create mileston button - Then milestone in each project should be created + Then group milestone should be created Scenario: I should see Issues listed with labels Given Group has projects with milestones diff --git a/features/steps/group/milestones.rb b/features/steps/group/milestones.rb index 7288dc87005464ce34cb89f82ee83318c9d528aa..915d766dd60284c5b4ec1c998ed36be3039c3652 100644 --- a/features/steps/group/milestones.rb +++ b/features/steps/group/milestones.rb @@ -54,14 +54,9 @@ class Spinach::Features::GroupMilestones < Spinach::FeatureSteps click_button "Create milestone" end - step 'milestone in each project should be created' do + step 'group milestone should be created' do group = Group.find_by(name: 'Owned') - expect(page).to have_content "Milestone v2.9.0" - expect(group.projects).to be_present - - group.projects.each do |project| - expect(page).to have_content project.name - end + expect(page).to have_content group.milestones.find_by_title('v2.9.0').title end step 'I should see the "bug" label' do diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 2c1d8278e0015c6c9b1ac225f201aecdfe20a983..8de56a4386b0d9c4210c53254b747adb23319282 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -284,7 +284,7 @@ class PersonalSnippet < Grape::Entity class ProjectEntity < Grape::Entity expose :id, :iid - expose(:project_id) { |entity| entity.project.id } + expose(:project_id) { |entity| entity&.project.try(:id) } expose :title, :description expose :state, :created_at, :updated_at end @@ -296,7 +296,12 @@ class RepoDiff < Grape::Entity expose :deleted_file?, as: :deleted_file end - class Milestone < ProjectEntity + class Milestone < Grape::Entity + expose :id, :iid + expose(:project_id) { |entity| entity&.project_id } + expose(:group_id) { |entity| entity&.group_id } + expose :title, :description + expose :state, :created_at, :updated_at expose :due_date expose :start_date end diff --git a/spec/controllers/groups/milestones_controller_spec.rb b/spec/controllers/groups/milestones_controller_spec.rb index c6e5fb61cf9f8bfd4660414fd21f510c07196884..aad67dd0164358a6cc2d3c00fd28b43d457f6770 100644 --- a/spec/controllers/groups/milestones_controller_spec.rb +++ b/spec/controllers/groups/milestones_controller_spec.rb @@ -2,8 +2,8 @@ describe Groups::MilestonesController do let(:group) { create(:group) } - let(:project) { create(:empty_project, group: group) } - let(:project2) { create(:empty_project, group: group) } + let!(:project) { create(:empty_project, group: group) } + let!(:project2) { create(:empty_project, group: group) } let(:user) { create(:user) } let(:title) { '肯定不是中文的问题' } let(:milestone) do @@ -17,24 +17,67 @@ end let(:milestone_path) { group_milestone_path(group, milestone.safe_title, title: milestone.title) } + let(:milestone_params) do + { + title: title, + start_date: Date.today, + due_date: 1.month.from_now.to_date + } + end + before do sign_in(user) group.add_owner(user) project.team << [user, :master] end - describe "#index" do + describe '#index' do it 'shows group milestones page' do get :index, group_id: group.to_param expect(response).to have_http_status(200) end - it 'shows group milestones JSON' do - get :index, group_id: group.to_param, format: :json + context 'as JSON' do + let!(:milestone) { create(:milestone, group: group, title: 'group milestone') } + let!(:legacy_milestone1) { create(:milestone, project: project, title: 'legacy') } + let!(:legacy_milestone2) { create(:milestone, project: project2, title: 'legacy') } - expect(response).to have_http_status(200) - expect(response.content_type).to eq 'application/json' + it 'lists legacy group milestones and group milestones' do + get :index, group_id: group.to_param, format: :json + + milestones = JSON.parse(response.body) + + expect(milestones.count).to eq(2) + expect(milestones.first["title"]).to eq("group milestone") + expect(milestones.second["title"]).to eq("legacy") + expect(response).to have_http_status(200) + expect(response.content_type).to eq 'application/json' + end + end + end + + describe '#show' do + let(:milestone1) { create(:milestone, project: project, title: 'legacy') } + let(:milestone2) { create(:milestone, project: project, title: 'legacy') } + let(:group_milestone) { create(:milestone, group: group) } + + context 'when there is a title parameter' do + it 'searchs for a legacy group milestone' do + expect(GlobalMilestone).to receive(:build) + expect(Milestone).not_to receive(:find_by_iid) + + get :show, group_id: group.to_param, id: title, title: milestone1.safe_title + end + end + + context 'when there is not a title parameter' do + it 'searchs for a group milestone' do + expect(GlobalMilestone).not_to receive(:build) + expect(Milestone).to receive(:find_by_iid) + + get :show, group_id: group.to_param, id: group_milestone.id + end end end @@ -44,16 +87,57 @@ it "creates group milestone with Chinese title" do post :create, group_id: group.to_param, - milestone: { project_ids: [project.id, project2.id], title: title } + milestone: milestone_params - expect(response).to redirect_to(group_milestone_path(group, title.to_slug.to_s, title: title)) - expect(Milestone.where(title: title).count).to eq(2) + milestone = Milestone.find_by_title(title) + + expect(response).to redirect_to(group_milestone_path(group, milestone.iid)) + expect(milestone.group_id).to eq(group.id) + expect(milestone.due_date).to eq(milestone_params[:due_date]) + expect(milestone.start_date).to eq(milestone_params[:start_date]) + end + end + + describe "#update" do + let(:milestone) { create(:milestone, group: group) } + + it "updates group milestone" do + milestone_params[:title] = "title changed" + + put :update, + id: milestone.iid, + group_id: group.to_param, + milestone: milestone_params + + milestone.reload + expect(response).to redirect_to(group_milestone_path(group, milestone.iid)) + expect(milestone.title).to eq("title changed") end - it "redirects to new when there are no project ids" do - post :create, group_id: group.to_param, milestone: { title: title, project_ids: [""] } - expect(response).to render_template :new - expect(assigns(:milestone).errors).not_to be_nil + context "legacy group milestones" do + let!(:milestone1) { create(:milestone, project: project, title: 'legacy milestone', description: "old description") } + let!(:milestone2) { create(:milestone, project: project2, title: 'legacy milestone', description: "old description") } + + it "updates only group milestones state" do + milestone_params[:title] = "title changed" + milestone_params[:description] = "description changed" + milestone_params[:state_event] = "close" + + put :update, + id: milestone1.title.to_slug.to_s, + group_id: group.to_param, + milestone: milestone_params, + title: milestone1.title + + expect(response).to redirect_to(group_milestone_path(group, milestone1.safe_title, title: milestone1.title)) + + [milestone1, milestone2].each do |milestone| + milestone.reload + expect(milestone.title).to eq("legacy milestone") + expect(milestone.description).to eq("old description") + expect(milestone.state).to eq("closed") + end + end end end @@ -156,7 +240,7 @@ it 'does not 404' do post :create, group_id: group.to_param, - milestone: { project_ids: [project.id, project2.id], title: title } + milestone: { title: title } expect(response).not_to have_http_status(404) end @@ -164,7 +248,7 @@ it 'does not redirect to the correct casing' do post :create, group_id: group.to_param, - milestone: { project_ids: [project.id, project2.id], title: title } + milestone: { title: title } expect(response).not_to have_http_status(301) end @@ -176,7 +260,7 @@ it 'returns not found' do post :create, group_id: redirect_route.path, - milestone: { project_ids: [project.id, project2.id], title: title } + milestone: { title: title } expect(response).to have_http_status(404) end diff --git a/spec/controllers/projects/milestones_controller_spec.rb b/spec/controllers/projects/milestones_controller_spec.rb index 84a61b2784eaeb127525011358b89878432f4cc2..bb5a340cd96d1b72e39fb88ce1ca56065393ef27 100644 --- a/spec/controllers/projects/milestones_controller_spec.rb +++ b/spec/controllers/projects/milestones_controller_spec.rb @@ -31,6 +31,40 @@ def view_milestone end end + describe "#index" do + context "as html" do + before do + get :index, namespace_id: project.namespace.id, project_id: project.id + end + + it "queries only projects milestones" do + milestones = assigns(:milestones) + + expect(milestones.count).to eq(1) + expect(milestones.where(project_id: nil)).to be_empty + end + end + + context "as json" do + let!(:group) { create(:group, :public) } + let!(:group_milestone) { create(:milestone, group: group) } + let!(:group_member) { create(:group_member, group: group, user: user) } + + before do + project.update(namespace: group) + get :index, namespace_id: project.namespace.id, project_id: project.id, format: :json + end + + it "queries projects milestones and groups milestones" do + milestones = assigns(:milestones) + + expect(milestones.count).to eq(2) + expect(milestones.where(project_id: nil).first).to eq(group_milestone) + expect(milestones.where(group_id: nil).first).to eq(milestone) + end + end + end + describe "#destroy" do it "removes milestone" do expect(issue.milestone_id).to eq(milestone.id) diff --git a/spec/factories/milestones.rb b/spec/factories/milestones.rb index 841ab3c73b86c43593c6d890e3033f4d279b5ff1..113665ff11b631ef59ebb1ff8d9065e5f7eeebc5 100644 --- a/spec/factories/milestones.rb +++ b/spec/factories/milestones.rb @@ -1,7 +1,13 @@ FactoryGirl.define do factory :milestone do title - project factory: :empty_project + + transient do + project nil + group nil + project_id nil + group_id nil + end trait :active do state "active" @@ -11,6 +17,20 @@ state "closed" end + after(:build) do |milestone, evaluator| + if evaluator.group + milestone.group = evaluator.group + elsif evaluator.group_id + milestone.group_id = evaluator.group_id + elsif evaluator.project + milestone.project = evaluator.project + elsif evaluator.project_id + milestone.project_id = evaluator.project_id + else + milestone.project = create(:empty_project) + end + end + factory :active_milestone, traits: [:active] factory :closed_milestone, traits: [:closed] end diff --git a/spec/features/groups/milestone_spec.rb b/spec/features/groups/milestone_spec.rb index 330310eae6b6a5601a365ed71ba0e6f529d59140..9b6eb946f4bcedce9f94fb3acc71e2fe72018164 100644 --- a/spec/features/groups/milestone_spec.rb +++ b/spec/features/groups/milestone_spec.rb @@ -33,4 +33,32 @@ expect(find('.start_date')).to have_content(Date.today.at_beginning_of_month.strftime('%b %-d, %Y')) end end + + context 'milestones list' do + let!(:other_project) { create(:project_empty_repo, group: group) } + + let!(:active_group_milestone) { create(:milestone, group: group, state: 'active') } + let!(:active_project_milestone1) { create(:milestone, project: project, state: 'active', title: 'v1.0') } + let!(:active_project_milestone2) { create(:milestone, project: other_project, state: 'active', title: 'v1.0') } + let!(:closed_group_milestone) { create(:milestone, group: group, state: 'closed') } + let!(:closed_project_milestone1) { create(:milestone, project: project, state: 'closed', title: 'v2.0') } + let!(:closed_project_milestone2) { create(:milestone, project: other_project, state: 'closed', title: 'v2.0') } + + before do + visit group_milestones_path(group) + end + + it 'counts milestones correctly' do + expect(find('.top-area .active .badge').text).to eq("2") + expect(find('.top-area .closed .badge').text).to eq("2") + expect(find('.top-area .all .badge').text).to eq("4") + end + + it 'lists legacy group milestones and group milestones' do + legacy_milestone = GroupMilestone.build_collection(group, group.projects, { state: 'active' }).first + + expect(page).to have_selector("#milestone_#{active_group_milestone.id}", count: 1) + expect(page).to have_selector("#milestone_#{legacy_milestone.milestones.first.id}", count: 1) + end + end end diff --git a/spec/features/milestone_spec.rb b/spec/features/milestone_spec.rb index 880c53343bcf28659f529a223e134383d527118c..2afdd0d321c78fabce27804908099aac0109b0d5 100644 --- a/spec/features/milestone_spec.rb +++ b/spec/features/milestone_spec.rb @@ -1,10 +1,12 @@ require 'rails_helper' feature 'Milestone', feature: true do - let(:project) { create(:empty_project, :public) } + let(:group) { create(:group, :public) } + let(:project) { create(:empty_project, :public, namespace: group) } let(:user) { create(:user) } before do + create(:group_member, group: group, user: user) project.team << [user, :master] gitlab_sign_in(user) end @@ -37,8 +39,8 @@ end end - feature 'Open a milestone with an existing title' do - scenario 'displays validation message' do + feature 'Open a project milestone with an existing title' do + scenario 'displays validation message when there is a project milestone with same title' do milestone = create(:milestone, project: project, title: 8.7) visit new_project_milestone_path(project) @@ -47,7 +49,20 @@ end find('input[name="commit"]').click - expect(find('.alert-danger')).to have_content('Title has already been taken') + expect(find('.alert-danger')).to have_content('already being used for another group or project milestone.') + end + + scenario 'displays validation message when there is a group milestone with same title' do + milestone = create(:milestone, project_id: nil, group: project.group, title: 8.7) + + visit new_group_milestone_path(project.group) + + page.within '.milestone-form' do + fill_in "milestone_title", with: milestone.title + end + find('input[name="commit"]').click + + expect(find('.alert-danger')).to have_content('already being used for another group or project milestone.') end end end diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index b9024d95df69e6f765b434befd2ff4a1417b7fa1..4ca0ac8a9b03fb94f1ef25a9a58a4b816dfbe745 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -75,6 +75,23 @@ end end + context 'filtering by group milestone' do + let!(:group) { create(:group, :public) } + let(:group_milestone) { create(:milestone, group: group) } + let!(:group_member) { create(:group_member, group: group, user: user) } + let(:params) { { milestone_title: group_milestone.title } } + + before do + project2.update(namespace: group) + issue2.update(milestone: group_milestone) + issue3.update(milestone: group_milestone) + end + + it 'returns issues assigned to that group milestone' do + expect(issues).to contain_exactly(issue2, issue3) + end + end + context 'filtering by no milestone' do let(:params) { { milestone_title: Milestone::None.title } } diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index 884870364a3e48d5826c5f120183ba92b83fea97..cc473397a1e46beadc56dadab1ad7f6031379ef0 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -53,6 +53,25 @@ expect(merge_requests).to contain_exactly(merge_request1) end + context 'filtering by group milestone' do + let!(:group) { create(:group, :public) } + let(:group_milestone) { create(:milestone, group: group) } + let!(:group_member) { create(:group_member, group: group, user: user) } + let(:params) { { milestone_title: group_milestone.title } } + + before do + project2.update(namespace: group) + merge_request2.update(milestone: group_milestone) + merge_request3.update(milestone: group_milestone) + end + + it 'returns issues assigned to that group milestone' do + merge_requests = described_class.new(user, params).execute + + expect(merge_requests).to contain_exactly(merge_request2, merge_request3) + end + end + context 'with created_after and created_before params' do let(:project4) { create(:empty_project, forked_from_project: project1) } diff --git a/spec/finders/milestones_finder_spec.rb b/spec/finders/milestones_finder_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..32ec983c5b8e57e699f3df97d734db2141213ecd --- /dev/null +++ b/spec/finders/milestones_finder_spec.rb @@ -0,0 +1,90 @@ +require 'spec_helper' + +describe MilestonesFinder do + let(:group) { create(:group) } + let(:project_1) { create(:empty_project, namespace: group) } + let(:project_2) { create(:empty_project, namespace: group) } + let!(:milestone_1) { create(:milestone, group: group, title: 'one test', due_date: Date.today) } + let!(:milestone_2) { create(:milestone, group: group) } + let!(:milestone_3) { create(:milestone, project: project_1, state: 'active', due_date: Date.tomorrow) } + let!(:milestone_4) { create(:milestone, project: project_2, state: 'active') } + + it 'it returns milestones for projects' do + result = described_class.new(project_ids: [project_1.id, project_2.id], state: 'all').execute + + expect(result).to contain_exactly(milestone_3, milestone_4) + end + + it 'returns milestones for groups' do + result = described_class.new(group_ids: group.id, state: 'all').execute + + expect(result).to contain_exactly(milestone_1, milestone_2) + end + + it 'returns milestones for groups and projects' do + result = described_class.new(project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all').execute + + expect(result).to contain_exactly(milestone_1, milestone_2, milestone_3, milestone_4) + end + + context 'with filters' do + let(:params) do + { + project_ids: [project_1.id, project_2.id], + group_ids: group.id, + state: 'all' + } + end + + before do + milestone_1.close + milestone_3.close + end + + it 'filters by active state' do + params[:state] = 'active' + result = described_class.new(params).execute + + expect(result).to contain_exactly(milestone_2, milestone_4) + end + + it 'filters by closed state' do + params[:state] = 'closed' + result = described_class.new(params).execute + + expect(result).to contain_exactly(milestone_1, milestone_3) + end + + it 'filters by title' do + result = described_class.new(params.merge(title: 'one test')).execute + + expect(result.to_a).to contain_exactly(milestone_1) + end + end + + context 'with order' do + let(:params) do + { + project_ids: [project_1.id, project_2.id], + group_ids: group.id, + state: 'all' + } + end + + it "default orders by due date" do + result = described_class.new(params).execute + + expect(result.first).to eq(milestone_1) + expect(result.second).to eq(milestone_3) + end + + it "orders by parameter" do + result = described_class.new(params.merge(order: 'id DESC')).execute + + expect(result.first).to eq(milestone_4) + expect(result.second).to eq(milestone_3) + expect(result.third).to eq(milestone_2) + expect(result.fourth).to eq(milestone_1) + end + end +end diff --git a/spec/fixtures/api/schemas/public_api/v3/issues.json b/spec/fixtures/api/schemas/public_api/v3/issues.json index 8ea4b3acf1369558adad12080f85eea59a708922..4dff996c96334aa85439c09b836aac7b95bed206 100644 --- a/spec/fixtures/api/schemas/public_api/v3/issues.json +++ b/spec/fixtures/api/schemas/public_api/v3/issues.json @@ -22,7 +22,8 @@ "properties": { "id": { "type": "integer" }, "iid": { "type": "integer" }, - "project_id": { "type": "integer" }, + "project_id": { "type": ["integer", "null"] }, + "group_id": { "type": ["integer", "null"] }, "title": { "type": "string" }, "description": { "type": ["string", "null"] }, "state": { "type": "string" }, diff --git a/spec/fixtures/api/schemas/public_api/v3/merge_requests.json b/spec/fixtures/api/schemas/public_api/v3/merge_requests.json index 09e33fba2602a1bbef779f0fdb9152b9c791ccca..06a12a3e09268918796caf2074811e52e26c8bdb 100644 --- a/spec/fixtures/api/schemas/public_api/v3/merge_requests.json +++ b/spec/fixtures/api/schemas/public_api/v3/merge_requests.json @@ -53,7 +53,8 @@ "properties": { "id": { "type": "integer" }, "iid": { "type": "integer" }, - "project_id": { "type": "integer" }, + "project_id": { "type": ["integer", "null"] }, + "group_id": { "type": ["integer", "null"] }, "title": { "type": "string" }, "description": { "type": ["string", "null"] }, "state": { "type": "string" }, diff --git a/spec/fixtures/api/schemas/public_api/v4/ee/issues.json b/spec/fixtures/api/schemas/public_api/v4/ee/issues.json index b2839c059d3e9ad9ed5735aca52ae5e7e3e06732..c38c5b56deb9a82a22bad255c8229760a2920664 100644 --- a/spec/fixtures/api/schemas/public_api/v4/ee/issues.json +++ b/spec/fixtures/api/schemas/public_api/v4/ee/issues.json @@ -23,6 +23,7 @@ "id": { "type": "integer" }, "iid": { "type": "integer" }, "project_id": { "type": "integer" }, + "group_id": { "type": ["integer", "null"] }, "title": { "type": "string" }, "description": { "type": ["string", "null"] }, "state": { "type": "string" }, diff --git a/spec/fixtures/api/schemas/public_api/v4/issues.json b/spec/fixtures/api/schemas/public_api/v4/issues.json index 2d1c84ee93d2130cbf8e689fb7cb564cd22f189c..bd6bfc03199e64a646f7f21e59232ba604324ea7 100644 --- a/spec/fixtures/api/schemas/public_api/v4/issues.json +++ b/spec/fixtures/api/schemas/public_api/v4/issues.json @@ -22,7 +22,8 @@ "properties": { "id": { "type": "integer" }, "iid": { "type": "integer" }, - "project_id": { "type": "integer" }, + "project_id": { "type": ["integer", "null"] }, + "group_id": { "type": ["integer", "null"] }, "title": { "type": "string" }, "description": { "type": ["string", "null"] }, "state": { "type": "string" }, diff --git a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json index 0d372e144e86330e7d581f95c3fc697e0be34869..ddda998f9611483f53918804722244648a4542fe 100644 --- a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json +++ b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json @@ -53,7 +53,8 @@ "properties": { "id": { "type": "integer" }, "iid": { "type": "integer" }, - "project_id": { "type": "integer" }, + "project_id": { "type": ["integer", "null"] }, + "group_id": { "type": ["integer", "null"] }, "title": { "type": "string" }, "description": { "type": ["string", "null"] }, "state": { "type": "string" }, diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 51e80e20a9d1d525a4d49dd06c520301fcf4afff..48f3db407ab11e404c106b26a347200fc82228a4 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -45,6 +45,7 @@ label: - merge_requests - priorities milestone: +- group - project - issues - labels diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 24d6838bc1b9537bed19016d73f323a28cf6a2cb..284608bdcb32fc202de2199edfe0bda2b66279cc 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -83,6 +83,7 @@ Milestone: - id - title - project_id +- group_id - description - due_date - start_date diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 85a1f02b27a7a2a61222be9bd7f6464155f9edec..d41e9af8d14ec7c4e29b743746596735d7a323a1 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -6,9 +6,6 @@ allow(subject).to receive(:set_iid).and_return(false) end - it { is_expected.to validate_presence_of(:title) } - it { is_expected.to validate_presence_of(:project) } - describe 'start_date' do it 'adds an error when start_date is greated then due_date' do milestone = build(:milestone, start_date: Date.tomorrow, due_date: Date.yesterday) @@ -39,17 +36,42 @@ end end - describe "unique milestone title per project" do - it "does not accept the same title in a project twice" do - new_milestone = Milestone.new(project: milestone.project, title: milestone.title) - expect(new_milestone).not_to be_valid + describe "unique milestone title" do + context "per project" do + it "does not accept the same title in a project twice" do + new_milestone = Milestone.new(project: milestone.project, title: milestone.title) + expect(new_milestone).not_to be_valid + end + + it "accepts the same title in another project" do + project = create(:empty_project) + new_milestone = Milestone.new(project: project, title: milestone.title) + + expect(new_milestone).to be_valid + end end - it "accepts the same title in another project" do - project = build(:empty_project) - new_milestone = Milestone.new(project: project, title: milestone.title) + context "per group" do + let(:group) { create(:group) } + let(:milestone) { create(:milestone, group: group) } + + before do + project.update(group: group) + end + + it "does not accept the same title in a group twice" do + new_milestone = Milestone.new(group: group, title: milestone.title) + + expect(new_milestone).not_to be_valid + end - expect(new_milestone).to be_valid + it "does not accept the same title of a child project milestone" do + create(:milestone, project: group.projects.first) + + new_milestone = Milestone.new(group: group, title: milestone.title) + + expect(new_milestone).not_to be_valid + end end end diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index d1dd1466d9570aaf2e363bc4b9fde564804f12f5..36d5038fb957b7d83e1d588d422ef9a92dd23501 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -37,9 +37,6 @@ describe '#execute' do shared_context 'issue move executed' do - let!(:milestone2) do - create(:milestone, project_id: new_project.id, title: 'v9.0') - end let!(:award_emoji) { create(:award_emoji, awardable: old_issue) } let!(:new_issue) { move_service.execute(old_issue, new_project) } @@ -48,6 +45,63 @@ context 'issue movable' do include_context 'user can move issue' + context 'move to new milestone' do + let(:new_issue) { move_service.execute(old_issue, new_project) } + + context 'project milestone' do + let!(:milestone2) do + create(:milestone, project_id: new_project.id, title: 'v9.0') + end + + it 'assigns milestone to new issue' do + expect(new_issue.reload.milestone.title).to eq 'v9.0' + expect(new_issue.reload.milestone).to eq(milestone2) + end + end + + context 'group milestones' do + let!(:group) { create(:group, :private) } + let!(:group_milestone_1) do + create(:milestone, group_id: group.id, title: 'v9.0_group') + end + + before do + old_issue.update(milestone: group_milestone_1) + old_project.update(namespace: group) + new_project.update(namespace: group) + + group.add_users([user], GroupMember::DEVELOPER) + end + + context 'when moving to a project of the same group' do + it 'keeps the same group milestone' do + expect(new_issue.reload.project).to eq(new_project) + expect(new_issue.reload.milestone).to eq(group_milestone_1) + end + end + + context 'when moving to a project of a different group' do + let!(:group_2) { create(:group, :private) } + + let!(:group_milestone_2) do + create(:milestone, group_id: group_2.id, title: 'v9.0_group') + end + + before do + old_issue.update(milestone: group_milestone_1) + new_project.update(namespace: group_2) + + group_2.add_users([user], GroupMember::DEVELOPER) + end + + it 'assigns to new group milestone of same title' do + expect(new_issue.reload.project).to eq(new_project) + expect(new_issue.reload.milestone).to eq(group_milestone_2) + end + end + end + end + context 'generic issue' do include_context 'issue move executed' @@ -55,11 +109,6 @@ expect(new_issue.project).to eq new_project end - it 'assigns milestone to new issue' do - expect(new_issue.reload.milestone.title).to eq 'v9.0' - expect(new_issue.reload.milestone).to eq(milestone2) - end - it 'assign labels to new issue' do expected_label_titles = new_issue.reload.labels.map(&:title) expect(expected_label_titles).to include 'label1' diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index c26642f5015c449e4f3a3077310f33975510a8dd..d0b991f19abbc9e3b27750eadff3a5e44e2b5548 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -253,13 +253,13 @@ def update_issue(opts) end context 'when the milestone change' do - before do + it 'marks todos as done' do update_issue(milestone: create(:milestone)) - end - it 'marks todos as done' do expect(todo.reload.done?).to eq true end + + it_behaves_like 'system notes for milestones' end context 'when the labels change' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index cb621635781adc9c0f82365e54fdc3480edcf589..2a1be13ac5d4badc044f68cbc9a825327f888b03 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -325,13 +325,13 @@ def update_merge_request(opts) end context 'when the milestone change' do - before do + it 'marks pending todos as done' do update_merge_request({ milestone: create(:milestone) }) - end - it 'marks pending todos as done' do expect(pending_todo.reload).to be_done end + + it_behaves_like 'system notes for milestones' end context 'when the labels change' do diff --git a/spec/support/issuable_shared_examples.rb b/spec/support/issuable_shared_examples.rb index 03011535351e0c1c56b11080eb20ed6afa613e78..970fe10db2b2062e9c1b98f86cb33ef046c6b23c 100644 --- a/spec/support/issuable_shared_examples.rb +++ b/spec/support/issuable_shared_examples.rb @@ -5,3 +5,34 @@ described_class.new(project, user, {}).execute(merge_request) end end + +shared_examples 'system notes for milestones' do + def update_issuable(opts) + issuable = try(:issue) || try(:merge_request) + described_class.new(project, user, opts).execute(issuable) + end + + context 'group milestones' do + let(:group) { create(:group) } + let(:group_milestone) { create(:milestone, group: group) } + + before do + project.update(namespace: group) + create(:group_member, group: group, user: user) + end + + it 'does not create system note' do + expect do + update_issuable(milestone: group_milestone) + end.not_to change { Note.system.count } + end + end + + context 'project milestones' do + it 'creates system note' do + expect do + update_issuable(milestone: create(:milestone)) + end.to change { Note.system.count }.by(1) + end + end +end