diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index faa4c8a5a4f2390c0933b728487237bcbb8ebd76..ec958e02e6b40c8aa2d9c72b5b63bac2b35efe52 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -2,6 +2,7 @@ module Issues class UpdateService < Issues::BaseService + prepend EE::Issues::UpdateService include SpamCheckService def execute(issue) diff --git a/app/services/milestones/update_service.rb b/app/services/milestones/update_service.rb index 81b20943babac7e3988aa33c95ec4a484c404e61..691081a4582ff148e9436425bfe9f906e3920822 100644 --- a/app/services/milestones/update_service.rb +++ b/app/services/milestones/update_service.rb @@ -2,6 +2,8 @@ module Milestones class UpdateService < Milestones::BaseService + prepend EE::Milestones::UpdateService + def execute(milestone) state = params[:state_event] diff --git a/db/schema.rb b/db/schema.rb index ad229d8717bdd6561c77c7a3ef4b1fe2955098a5..27717e0fb65fac336c77e835be7ef50e9f8e6bcc 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -954,6 +954,12 @@ t.string "title_html", null: false t.text "description" t.text "description_html" + t.integer "start_date_sourcing_milestone_id" + t.integer "due_date_sourcing_milestone_id" + t.date "start_date_fixed" + t.date "due_date_fixed" + t.boolean "start_date_is_fixed" + t.boolean "due_date_is_fixed" end add_index "epics", ["assignee_id"], name: "index_epics_on_assignee_id", using: :btree diff --git a/doc/api/epics.md b/doc/api/epics.md index 502e327a3bbb5ea879225427ecb2ce9315d8977b..96aa42a27a5b9c1c874834f9b3e1b0107df3498a 100644 --- a/doc/api/epics.md +++ b/doc/api/epics.md @@ -10,6 +10,14 @@ If epics feature is not available a `403` status code will be returned. The [epic issues API](epic_issues.md) allows you to interact with issues associated with an epic. +# Milestone dates integration + +> [Introduced][ee-6448] in GitLab 11.2. + +Since start date and due date can be dynamically sourced from related issue milestones, when user has edit permission, additional fields will be shown. These include two boolean fields `start_date_is_fixed` and `due_date_is_fixed`, and four date fields `start_date_fixed`, `start_date_from_milestones`, `due_date_fixed` and `due_date_from_milestones`. + +`end_date` has been deprecated in favor of `due_date`. + ## List epics for a group Gets all epics of the requested group and its subgroups. @@ -51,11 +59,18 @@ Example response: "avatar_url": "http://www.gravatar.com/avatar/018729e129a6f31c80a6327a30196823?s=80&d=identicon", "web_url": "http://localhost:3001/kam" }, - "labels": [], "start_date": null, - "end_date": null, - "created_at": "2018-01-21T06:21:13.165Z", - "updated_at": "2018-01-22T12:41:41.166Z" + "start_date_is_fixed": false, + "start_date_fixed": null, + "start_date_from_milestones": null, + "end_date": "2018-07-31", + "due_date": "2018-07-31", + "due_date_is_fixed": false, + "due_date_fixed": null, + "due_date_from_milestones": "2018-07-31", + "created_at": "2018-07-17T13:36:22.770Z", + "updated_at": "2018-07-18T12:22:05.239Z", + "labels": [] } ] ``` @@ -95,9 +110,17 @@ Example response: "web_url": "http://localhost:3001/arnita" }, "start_date": null, - "end_date": null, - "created_at": "2018-01-21T06:21:13.165Z", - "updated_at": "2018-01-22T12:41:41.166Z" + "start_date_is_fixed": false, + "start_date_fixed": null, + "start_date_from_milestones": null, + "end_date": "2018-07-31", + "due_date": "2018-07-31", + "due_date_is_fixed": false, + "due_date_fixed": null, + "due_date_from_milestones": "2018-07-31", + "created_at": "2018-07-17T13:36:22.770Z", + "updated_at": "2018-07-18T12:22:05.239Z", + "labels": [] } ``` @@ -139,11 +162,18 @@ Example response: "id" : 18, "username" : "eileen.lowe" }, - "labels": [], "start_date": null, - "end_date": null, - "created_at": "2018-01-21T06:21:13.165Z", - "updated_at": "2018-01-22T12:41:41.166Z" + "start_date_is_fixed": false, + "start_date_fixed": null, + "start_date_from_milestones": null, + "end_date": "2018-07-31", + "due_date": "2018-07-31", + "due_date_is_fixed": false, + "due_date_fixed": null, + "due_date_from_milestones": "2018-07-31", + "created_at": "2018-07-17T13:36:22.770Z", + "updated_at": "2018-07-18T12:22:05.239Z", + "labels": [] } ``` @@ -151,6 +181,8 @@ Example response: Updates an epic +Note that after 11.2, `start_date` and `end_date` should no longer be updated directly, as they are now composite fields. User can configure the `_is_fixed` and `_fixed` fields instead. + ``` PUT /groups/:id/epics/:epic_iid ``` @@ -162,8 +194,10 @@ PUT /groups/:id/epics/:epic_iid | `title` | string | no | The title of an epic | | `description` | string | no | The description of an epic | | `labels` | string | no | The comma separated list of labels | -| `start_date` | string | no | The start date of an epic | -| `end_date` | string. | no | The end date of an epic | +| `start_date_is_fixed` | boolean | no | Whether start date should be sourced from `start_date_fixed` | +| `start_date_fixed` | string | no | The fixed start date of an epic | +| `due_date_is_fixed` | boolean | no | Whether due date should be sourced from `due_date_fixed` | +| `due_date_fixed` | string | no | The fixed due date of an epic | ```bash curl --header PUT "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/groups/1/epics/5?title=New%20Title @@ -186,11 +220,18 @@ Example response: "id" : 18, "username" : "eileen.lowe" }, - "labels": [], "start_date": null, - "end_date": null, - "created_at": "2018-01-21T06:21:13.165Z", - "updated_at": "2018-01-22T12:41:41.166Z" + "start_date_is_fixed": false, + "start_date_fixed": null, + "start_date_from_milestones": null, + "end_date": "2018-07-31", + "due_date": "2018-07-31", + "due_date_is_fixed": false, + "due_date_fixed": null, + "due_date_from_milestones": "2018-07-31", + "created_at": "2018-07-17T13:36:22.770Z", + "updated_at": "2018-07-18T12:22:05.239Z", + "labels": [] } ``` @@ -278,3 +319,5 @@ Example response: "created_at": "2016-07-01T11:09:13.992Z" } ``` + +[ee-6448]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6448 diff --git a/ee/app/controllers/groups/epics_controller.rb b/ee/app/controllers/groups/epics_controller.rb index 6d13b87dd05ed96ac08248eefc76bd2d2b68e33b..dbf81689f36deafcb9258d0a582cdc4578b99c25 100644 --- a/ee/app/controllers/groups/epics_controller.rb +++ b/ee/app/controllers/groups/epics_controller.rb @@ -66,8 +66,10 @@ def epic_params_attributes [ :title, :description, - :start_date, - :end_date, + :start_date_fixed, + :start_date_is_fixed, + :due_date_fixed, + :due_date_is_fixed, label_ids: [] ] end diff --git a/ee/app/finders/epics/date_sourcing_milestones_finder.rb b/ee/app/finders/epics/date_sourcing_milestones_finder.rb new file mode 100644 index 0000000000000000000000000000000000000000..5ff63dbd5576bcf14acc42254d23df2c78127a7b --- /dev/null +++ b/ee/app/finders/epics/date_sourcing_milestones_finder.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +module Epics + class DateSourcingMilestonesFinder + include Gitlab::Utils::StrongMemoize + + FIELDS = [:id, :start_date, :due_date].freeze + ID_INDEX = FIELDS.index(:id) + START_DATE_INDEX = FIELDS.index(:start_date) + DUE_DATE_INDEX = FIELDS.index(:due_date) + + def initialize(epic_id) + @epic_id = epic_id + end + + def execute + strong_memoize(:execute) do + Milestone.joins(issues: :epic_issue).where(epic_issues: { epic_id: epic_id }).joins( + <<~SQL + INNER JOIN ( + SELECT MIN(milestones.start_date) AS start_date, MAX(milestones.due_date) AS due_date + FROM milestones + INNER JOIN issues ON issues.milestone_id = milestones.id + INNER JOIN epic_issues ON epic_issues.issue_id = issues.id + WHERE epic_issues.epic_id = #{epic_id} + ) inner_results ON (inner_results.start_date = milestones.start_date OR inner_results.due_date = milestones.due_date) + SQL + ).pluck(*FIELDS) + end + end + + def start_date + start_date_sourcing_milestone&.slice(START_DATE_INDEX) + end + + def start_date_sourcing_milestone_id + start_date_sourcing_milestone&.slice(ID_INDEX) + end + + def due_date + due_date_sourcing_milestone&.slice(DUE_DATE_INDEX) + end + + def due_date_sourcing_milestone_id + due_date_sourcing_milestone&.slice(ID_INDEX) + end + + private + + attr_reader :epic_id + + def start_date_sourcing_milestone + @start_date_sourcing_milestone ||= execute + .reject { |row| row[START_DATE_INDEX].nil? } + .min_by { |row| row[START_DATE_INDEX] } + end + + def due_date_sourcing_milestone + @due_date_sourcing_milestone ||= execute + .reject { |row| row[DUE_DATE_INDEX].nil? } + .max_by { |row| row[DUE_DATE_INDEX] } + end + end +end diff --git a/ee/app/helpers/epics_helper.rb b/ee/app/helpers/epics_helper.rb index 46992fc18b17926feda2fb9789829b2348993d4a..97789c3c7380add078e611252e9c842eb151861c 100644 --- a/ee/app/helpers/epics_helper.rb +++ b/ee/app/helpers/epics_helper.rb @@ -16,11 +16,25 @@ def epic_show_app_data(epic, opts) todo_exists: todo.present?, todo_path: group_todos_path(group), start_date: epic.start_date, + due_date: epic.due_date, end_date: epic.end_date } epic_meta[:todo_delete_path] = dashboard_todo_path(todo) if todo.present? + if Ability.allowed?(current_user, :update_epic, epic.group) + epic_meta.merge!( + start_date_fixed: epic.start_date_fixed, + start_date_is_fixed: epic.start_date_is_fixed?, + start_date_from_milestones: epic.start_date_from_milestones, + start_date_sourcing_milestone_title: epic.start_date_sourcing_milestone&.title, + due_date_fixed: epic.due_date_fixed, + due_date_is_fixed: epic.due_date_is_fixed?, + due_date_from_milestones: epic.due_date_from_milestones, + due_date_sourcing_milestone_title: epic.due_date_sourcing_milestone&.title + ) + end + participants = UserSerializer.new.represent(epic.participants) initial = opts[:initial].merge(labels: epic.labels, participants: participants, diff --git a/ee/app/models/ee/epic.rb b/ee/app/models/ee/epic.rb index bc9f6686fcc6683c8324bb635ca7c5acab0b5a3e..65a63316c6e57f2f2dc658d2b248149ff0d3a9d0 100644 --- a/ee/app/models/ee/epic.rb +++ b/ee/app/models/ee/epic.rb @@ -13,10 +13,13 @@ module Epic belongs_to :assignee, class_name: "User" belongs_to :group + belongs_to :start_date_sourcing_milestone, class_name: 'Milestone' + belongs_to :due_date_sourcing_milestone, class_name: 'Milestone' has_internal_id :iid, scope: :group, init: ->(s) { s&.group&.epics&.maximum(:iid) } has_many :epic_issues + has_many :issues, through: :epic_issues validates :group, presence: true @@ -78,6 +81,36 @@ def order_by(method) def parent_class ::Group end + + def update_start_and_due_dates(epics) + groups = epics.includes(:issues).group_by do |epic| + milestone_ids = epic.issues.map(&:milestone_id) + milestone_ids.compact! + milestone_ids.uniq! + milestone_ids + end + + groups.each do |milestone_ids, epics| + next if milestone_ids.empty? + + results = Epics::DateSourcingMilestonesFinder.new(epics.first.id) + + self.where(id: epics.map(&:id)).update_all( + [ + %{ + start_date = CASE WHEN start_date_is_fixed = true THEN start_date ELSE ? END, + start_date_sourcing_milestone_id = ?, + end_date = CASE WHEN due_date_is_fixed = true THEN end_date ELSE ? END, + due_date_sourcing_milestone_id = ? + }, + results.start_date, + results.start_date_sourcing_milestone_id, + results.due_date, + results.due_date_sourcing_milestone_id + ] + ) + end + end end def assignees @@ -109,6 +142,25 @@ def elapsed_days # Needed to use EntityDateHelper#remaining_days_in_words alias_attribute(:due_date, :end_date) + def update_start_and_due_dates + results = Epics::DateSourcingMilestonesFinder.new(id) + + self.start_date = start_date_is_fixed? ? start_date_fixed : results.start_date + self.start_date_sourcing_milestone_id = results.start_date_sourcing_milestone_id + self.due_date = due_date_is_fixed? ? due_date_fixed : results.due_date + self.due_date_sourcing_milestone_id = results.due_date_sourcing_milestone_id + + save if changed? + end + + def start_date_from_milestones + start_date_is_fixed? ? start_date_sourcing_milestone&.start_date : start_date + end + + def due_date_from_milestones + due_date_is_fixed? ? due_date_sourcing_milestone&.due_date : due_date + end + def to_reference(from = nil, full: false) reference = "#{self.class.reference_prefix}#{iid}" @@ -125,7 +177,7 @@ def cross_reference?(from) def update_project_counter_caches end - def issues(current_user) + def issues_readable_by(current_user) related_issues = ::Issue.select('issues.*, epic_issues.id as epic_issue_id, epic_issues.relative_position') .joins(:epic_issue) .where("epic_issues.epic_id = #{id}") diff --git a/ee/app/serializers/epic_entity.rb b/ee/app/serializers/epic_entity.rb index 031e5a618f7751441540701342fa9ed814b309dc..68fa5e82be29b9b8e4c01bc224f9b2275c3cdf90 100644 --- a/ee/app/serializers/epic_entity.rb +++ b/ee/app/serializers/epic_entity.rb @@ -6,8 +6,15 @@ class EpicEntity < IssuableEntity expose :group_full_name do |epic| epic.group.full_name end + expose :start_date - expose :end_date + expose :start_date_is_fixed?, as: :start_date_is_fixed + expose :start_date_fixed, :start_date_from_milestones + expose :end_date # @deprecated + expose :end_date, as: :due_date + expose :due_date_is_fixed?, as: :due_date_is_fixed + expose :due_date_fixed, :due_date_from_milestones + expose :web_url do |epic| group_epic_path(epic.group, epic) end diff --git a/ee/app/services/ee/issues/update_service.rb b/ee/app/services/ee/issues/update_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..5887b349d1faf957653428e15133e495a36ac0cb --- /dev/null +++ b/ee/app/services/ee/issues/update_service.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module EE + module Issues + module UpdateService + extend ::Gitlab::Utils::Override + + override :execute + def execute(issue) + result = super + + if issue.previous_changes.include?(:milestone_id) && issue.epic + issue.epic.update_start_and_due_dates + end + + result + end + end + end +end diff --git a/ee/app/services/ee/milestones/update_service.rb b/ee/app/services/ee/milestones/update_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..4b8152caf55a0041711b3a8c475ea058e270efca --- /dev/null +++ b/ee/app/services/ee/milestones/update_service.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module EE + module Milestones + module UpdateService + extend ::Gitlab::Utils::Override + + override :execute + def execute(milestone) + super + + if dates_changed?(milestone) + ::Epic.update_start_and_due_dates( + ::Epic.joins(:issues).where(issues: { milestone_id: milestone.id }) + ) + end + + milestone + end + + private + + def dates_changed?(milestone) + changes = milestone.previous_changes + changes.include?(:start_date) || changes.include?(:due_date) + end + end + end +end diff --git a/ee/app/services/epic_issues/create_service.rb b/ee/app/services/epic_issues/create_service.rb index 5a9129ee1c340d04d732f97b7dec411800b78a76..1bfe62e6f6633a60920bf9fc68b0067531f284af 100644 --- a/ee/app/services/epic_issues/create_service.rb +++ b/ee/app/services/epic_issues/create_service.rb @@ -1,5 +1,11 @@ module EpicIssues class CreateService < IssuableLinks::CreateService + def execute + result = super + issuable.update_start_and_due_dates + result + end + private def relate_issues(referenced_issue) diff --git a/ee/app/services/epic_issues/destroy_service.rb b/ee/app/services/epic_issues/destroy_service.rb index 5440b9505ecdd6263baf642eb4529b4f082d4d04..ea6a441ddb6f5072cfb63229070e8daa0fbdc657 100644 --- a/ee/app/services/epic_issues/destroy_service.rb +++ b/ee/app/services/epic_issues/destroy_service.rb @@ -1,5 +1,11 @@ module EpicIssues class DestroyService < IssuableLinks::DestroyService + def execute + result = super + link.epic.update_start_and_due_dates + result + end + private def source diff --git a/ee/app/services/epic_issues/list_service.rb b/ee/app/services/epic_issues/list_service.rb index 45535bec9e7f86ec9724bb52b39fed6bd5741c2a..90628c82b888c423ae18c6c933764ff4cdd912da 100644 --- a/ee/app/services/epic_issues/list_service.rb +++ b/ee/app/services/epic_issues/list_service.rb @@ -5,7 +5,7 @@ class ListService < IssuableLinks::ListService def issues return [] unless issuable&.group&.feature_available?(:epics) - issuable.issues(current_user) + issuable.issues_readable_by(current_user) end def relation_path(issue) diff --git a/ee/app/services/epics/update_service.rb b/ee/app/services/epics/update_service.rb index 6519da04febadb04ca1adb555079a01e43db4e1b..485b468e0f279d4cce83e21367c1b8cde2143a4d 100644 --- a/ee/app/services/epics/update_service.rb +++ b/ee/app/services/epics/update_service.rb @@ -1,8 +1,21 @@ module Epics class UpdateService < Epics::BaseService + EPIC_DATE_FIELDS = %I[ + start_date_fixed + start_date_is_fixed + due_date_fixed + due_date_is_fixed + ].freeze + def execute(epic) + # start_date and end_date columns are no longer writable by users because those + # are composite fields managed by the system. + params.except!(:start_date, :end_date) + update(epic) + epic.update_start_and_due_dates if have_epic_dates_changed?(epic) + epic end @@ -17,5 +30,11 @@ def handle_changes(epic, options) todo_service.update_epic(epic, current_user, old_mentioned_users) end + + private + + def have_epic_dates_changed?(epic) + (epic.previous_changes.keys.map(&:to_sym) & EPIC_DATE_FIELDS).present? + end end end diff --git a/ee/changelogs/unreleased/6470-milestone-dates-integrated-into-epics.yml b/ee/changelogs/unreleased/6470-milestone-dates-integrated-into-epics.yml new file mode 100644 index 0000000000000000000000000000000000000000..c7064e677692de2c43edf8bebde089c72ccd9d70 --- /dev/null +++ b/ee/changelogs/unreleased/6470-milestone-dates-integrated-into-epics.yml @@ -0,0 +1,5 @@ +--- +title: Allow epic start/due dates to be sourceable from issue milestones +merge_request: 6470 +author: +type: added diff --git a/ee/db/migrate/20180711014025_add_date_columns_to_epics.rb b/ee/db/migrate/20180711014025_add_date_columns_to_epics.rb new file mode 100644 index 0000000000000000000000000000000000000000..ae4e0ac31606cbb6c1031fd5eb4565800fe7265c --- /dev/null +++ b/ee/db/migrate/20180711014025_add_date_columns_to_epics.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class AddDateColumnsToEpics < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + change_table :epics do |t| + t.references :start_date_sourcing_milestone + t.references :due_date_sourcing_milestone + t.date :start_date_fixed + t.date :due_date_fixed + t.boolean :start_date_is_fixed + t.boolean :due_date_is_fixed + end + end +end diff --git a/ee/db/migrate/20180711014026_update_date_columns_on_epics.rb b/ee/db/migrate/20180711014026_update_date_columns_on_epics.rb new file mode 100644 index 0000000000000000000000000000000000000000..c39531940c13807726e2f5682227bf21f7f415f3 --- /dev/null +++ b/ee/db/migrate/20180711014026_update_date_columns_on_epics.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class UpdateDateColumnsOnEpics < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + disable_ddl_transaction! + + class Epic < ActiveRecord::Base + self.table_name = 'epics' + include EachBatch + end + + def up + Epic.where.not(start_date: nil).each_batch do |batch| + batch.update_all('start_date_is_fixed = true, start_date_fixed = start_date') + end + + Epic.where.not(end_date: nil).each_batch do |batch| + batch.update_all('due_date_is_fixed = true, due_date_fixed = end_date') + end + end + + def down + # NOOP + end +end diff --git a/ee/db/post_migrate/20180713171825_update_epic_dates_from_milestones.rb b/ee/db/post_migrate/20180713171825_update_epic_dates_from_milestones.rb new file mode 100644 index 0000000000000000000000000000000000000000..d71840b3d7bed0919be2a62e02384371143b4fbe --- /dev/null +++ b/ee/db/post_migrate/20180713171825_update_epic_dates_from_milestones.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +class UpdateEpicDatesFromMilestones < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + class Epic < ActiveRecord::Base + self.table_name = 'epics' + include EachBatch + + has_many :epic_issues + has_many :issues, through: :epic_issues + + def self.update_start_and_due_dates(epics) + groups = epics.includes(:issues).group_by do |epic| + milestone_ids = epic.issues.map(&:milestone_id) + milestone_ids.compact! + milestone_ids.uniq! + milestone_ids + end + + groups.each do |milestone_ids, epics| + next if milestone_ids.empty? + + data = ::UpdateEpicDatesFromMilestones::Epics::DateSourcingMilestonesFinder.new(epics.first.id) + + self.where(id: epics.map(&:id)).update_all( + [ + %{ + start_date = CASE WHEN start_date_is_fixed = true THEN start_date ELSE ? END, + start_date_sourcing_milestone_id = ?, + end_date = CASE WHEN due_date_is_fixed = true THEN end_date ELSE ? END, + due_date_sourcing_milestone_id = ? + }, + data.start_date, + data.start_date_sourcing_milestone_id, + data.due_date, + data.due_date_sourcing_milestone_id + ] + ) + end + end + end + + module Epics + class DateSourcingMilestonesFinder + include Gitlab::Utils::StrongMemoize + + FIELDS = [:id, :start_date, :due_date].freeze + ID_INDEX = FIELDS.index(:id) + START_DATE_INDEX = FIELDS.index(:start_date) + DUE_DATE_INDEX = FIELDS.index(:due_date) + + def initialize(epic_id) + @epic_id = epic_id + end + + def execute + strong_memoize(:execute) do + Milestone.joins(issues: :epic_issue).where(epic_issues: { epic_id: epic_id }).joins( + <<~SQL + INNER JOIN ( + SELECT MIN(milestones.start_date) AS start_date, MAX(milestones.due_date) AS due_date + FROM milestones + INNER JOIN issues ON issues.milestone_id = milestones.id + INNER JOIN epic_issues ON epic_issues.issue_id = issues.id + WHERE epic_issues.epic_id = #{epic_id} + ) inner_results ON (inner_results.start_date = milestones.start_date OR inner_results.due_date = milestones.due_date) + SQL + ).pluck(*FIELDS) + end + end + + def start_date + start_date_sourcing_milestone&.slice(START_DATE_INDEX) + end + + def start_date_sourcing_milestone_id + start_date_sourcing_milestone&.slice(ID_INDEX) + end + + def due_date + due_date_sourcing_milestone&.slice(DUE_DATE_INDEX) + end + + def due_date_sourcing_milestone_id + due_date_sourcing_milestone&.slice(ID_INDEX) + end + + private + + attr_reader :epic_id + + def start_date_sourcing_milestone + @start_date_sourcing_milestone ||= execute + .reject { |row| row[START_DATE_INDEX].nil? } + .min_by { |row| row[START_DATE_INDEX] } + end + + def due_date_sourcing_milestone + @due_date_sourcing_milestone ||= execute + .reject { |row| row[DUE_DATE_INDEX].nil? } + .max_by { |row| row[DUE_DATE_INDEX] } + end + end + end + + def up + # Fill fixed date columns for remaining eligible records touched after regular migration is run + # (20180711014026_update_date_columns_on_epics) but before new app code takes effect. + Epic.where(start_date_is_fixed: nil).where.not(start_date: nil).each_batch do |batch| + batch.update_all('start_date_is_fixed = true, start_date_fixed = start_date') + end + Epic.where(due_date_is_fixed: nil).where.not(end_date: nil).each_batch do |batch| + batch.update_all('due_date_is_fixed = true, due_date_fixed = end_date') + end + + Epic.joins(:issues).where('issues.milestone_id IS NOT NULL').each_batch do |epics| + Epic.update_start_and_due_dates(epics) + end + end + + def down + # NOOP + end +end diff --git a/ee/lib/api/epic_issues.rb b/ee/lib/api/epic_issues.rb index 2c01dd6e1dd5558a9d7752fcaa72e2acd74dc8d8..7db1540aa738a09976425a76ed041be0559b334b 100644 --- a/ee/lib/api/epic_issues.rb +++ b/ee/lib/api/epic_issues.rb @@ -53,7 +53,7 @@ def link # For now we return empty body # The issues list in the correct order in body will be returned as part of #4250 if result - present epic.issues(current_user), + present epic.issues_readable_by(current_user), with: EE::API::Entities::EpicIssue, current_user: current_user else @@ -70,7 +70,7 @@ def link get ':id/(-/)epics/:epic_iid/issues' do authorize_can_read! - present epic.issues(current_user), + present epic.issues_readable_by(current_user), with: EE::API::Entities::EpicIssue, current_user: current_user end diff --git a/ee/lib/api/epics.rb b/ee/lib/api/epics.rb index 4008688d60ccdad297b2124cd93a6f68df614671..8625c087edd93e7663a6dc65cc9700aa3dbb9a70 100644 --- a/ee/lib/api/epics.rb +++ b/ee/lib/api/epics.rb @@ -58,7 +58,7 @@ def find_epics(args = {}) optional :labels, type: String, desc: 'Comma-separated list of label names' end get ':id/(-/)epics' do - present find_epics(group_id: user_group.id), with: EE::API::Entities::Epic + present find_epics(group_id: user_group.id), with: EE::API::Entities::Epic, user: current_user end desc 'Get details of an epic' do @@ -70,7 +70,7 @@ def find_epics(args = {}) get ':id/(-/)epics/:epic_iid' do authorize_can_read! - present epic, with: EE::API::Entities::Epic + present epic, with: EE::API::Entities::Epic, user: current_user end desc 'Create a new epic' do @@ -79,8 +79,10 @@ def find_epics(args = {}) params do requires :title, type: String, desc: 'The title of an epic' optional :description, type: String, desc: 'The description of an epic' - optional :start_date, type: String, desc: 'The start date of an epic' - optional :end_date, type: String, desc: 'The end date of an epic' + optional :start_date, as: :start_date_fixed, type: String, desc: 'The start date of an epic' + optional :start_date_is_fixed, type: Boolean, desc: 'Indicates start date should be sourced from start_date_fixed field not the issue milestones' + optional :end_date, as: :due_date_fixed, type: String, desc: 'The due date of an epic' + optional :due_date_is_fixed, type: Boolean, desc: 'Indicates due date should be sourced from due_date_fixed field not the issue milestones' optional :labels, type: String, desc: 'Comma-separated list of label names' end post ':id/(-/)epics' do @@ -88,7 +90,7 @@ def find_epics(args = {}) epic = ::Epics::CreateService.new(user_group, current_user, declared_params(include_missing: false)).execute if epic.valid? - present epic, with: EE::API::Entities::Epic + present epic, with: EE::API::Entities::Epic, user: current_user else render_validation_error!(epic) end @@ -101,10 +103,12 @@ def find_epics(args = {}) requires :epic_iid, type: Integer, desc: 'The internal ID of an epic' optional :title, type: String, desc: 'The title of an epic' optional :description, type: String, desc: 'The description of an epic' - optional :start_date, type: String, desc: 'The start date of an epic' - optional :end_date, type: String, desc: 'The end date of an epic' + optional :start_date, as: :start_date_fixed, type: String, desc: 'The start date of an epic' + optional :start_date_is_fixed, type: Boolean, desc: 'Indicates start date should be sourced from start_date_fixed field not the issue milestones' + optional :end_date, as: :due_date_fixed, type: String, desc: 'The due date of an epic' + optional :due_date_is_fixed, type: Boolean, desc: 'Indicates due date should be sourced from due_date_fixed field not the issue milestones' optional :labels, type: String, desc: 'Comma-separated list of label names' - at_least_one_of :title, :description, :start_date, :end_date, :labels + at_least_one_of :title, :description, :start_date_fixed, :due_date_fixed, :labels end put ':id/(-/)epics/:epic_iid' do authorize_can_admin! @@ -114,7 +118,7 @@ def find_epics(args = {}) result = ::Epics::UpdateService.new(user_group, current_user, update_params).execute(epic) if result.valid? - present result, with: EE::API::Entities::Epic + present result, with: EE::API::Entities::Epic, user: current_user else render_validation_error!(result) end diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb index 816e967ab63f107c17dab53fd81cf05d4af39865..75bdcb3fad8e0ad0a2a2f42c2c39315bb16a19f5 100644 --- a/ee/lib/ee/api/entities.rb +++ b/ee/lib/ee/api/entities.rb @@ -155,6 +155,8 @@ class RelatedIssue < ::API::Entities::Issue end class Epic < Grape::Entity + can_admin_epic = ->(epic, opts) { Ability.allowed?(opts[:user], :admin_epic, epic) } + expose :id expose :iid expose :group_id @@ -162,7 +164,12 @@ class Epic < Grape::Entity expose :description expose :author, using: ::API::Entities::UserBasic expose :start_date - expose :end_date + expose :start_date_is_fixed?, as: :start_date_is_fixed, if: can_admin_epic + expose :start_date_fixed, :start_date_from_milestones, if: can_admin_epic + expose :end_date # @deprecated + expose :end_date, as: :due_date + expose :due_date_is_fixed?, as: :due_date_is_fixed, if: can_admin_epic + expose :due_date_fixed, :due_date_from_milestones, if: can_admin_epic expose :created_at expose :updated_at expose :labels do |epic, options| diff --git a/ee/spec/controllers/groups/epics_controller_spec.rb b/ee/spec/controllers/groups/epics_controller_spec.rb index f29d1217b5a1f8ab94ccd12ca078f3dcb5a0759d..102ea787e0a587fb6d33c6ee0363623dfae5f00c 100644 --- a/ee/spec/controllers/groups/epics_controller_spec.rb +++ b/ee/spec/controllers/groups/epics_controller_spec.rb @@ -181,9 +181,11 @@ def show_epic(format = :html) end describe 'PUT #update' do + let(:date) { Date.new(2002, 1, 1)} + before do group.add_developer(user) - put :update, group_id: group, id: epic.to_param, epic: { title: 'New title', label_ids: [label.id] }, format: :json + put :update, group_id: group, id: epic.to_param, epic: { title: 'New title', label_ids: [label.id], start_date_fixed: '2002-01-01', start_date_is_fixed: true }, format: :json end it 'returns status 200' do @@ -195,6 +197,9 @@ def show_epic(format = :html) expect(epic.title).to eq('New title') expect(epic.labels).to eq([label]) + expect(epic.start_date_fixed).to eq(date) + expect(epic.start_date).to eq(date) + expect(epic.start_date_is_fixed).to eq(true) end end diff --git a/ee/spec/factories/epics.rb b/ee/spec/factories/epics.rb index 022e1dc95e022937cce32d18b4f5163fed22267c..eb5a62b1909d181ad0cd0f118a0aeeebb1c2c670 100644 --- a/ee/spec/factories/epics.rb +++ b/ee/spec/factories/epics.rb @@ -4,6 +4,15 @@ group author + trait :use_fixed_dates do + start_date { Date.new(2010, 1, 1) } + start_date_fixed { Date.new(2010, 1, 1) } + start_date_is_fixed true + end_date { Date.new(2010, 1, 3) } + due_date_fixed { Date.new(2010, 1, 3) } + due_date_is_fixed true + end + factory :labeled_epic do transient do labels [] diff --git a/ee/spec/finders/epics/date_sourcing_milestones_finder_spec.rb b/ee/spec/finders/epics/date_sourcing_milestones_finder_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..1bdb88dbb87644c88a0280d9340e7c10f15a7b95 --- /dev/null +++ b/ee/spec/finders/epics/date_sourcing_milestones_finder_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Epics::DateSourcingMilestonesFinder do + describe '#execute' do + it 'returns date and id from milestones' do + epic = create(:epic) + milestone1 = create(:milestone, start_date: Date.new(2000, 1, 1), due_date: Date.new(2000, 1, 10)) + milestone2 = create(:milestone, due_date: Date.new(2000, 1, 30)) + milestone3 = create(:milestone, start_date: Date.new(2000, 1, 1), due_date: Date.new(2000, 1, 20)) + create(:issue, epic: epic, milestone: milestone1) + create(:issue, epic: epic, milestone: milestone2) + create(:issue, epic: epic, milestone: milestone3) + + results = described_class.new(epic.id) + + expect(results).to have_attributes( + start_date: milestone1.start_date, + start_date_sourcing_milestone_id: milestone1.id, + due_date: milestone2.due_date, + due_date_sourcing_milestone_id: milestone2.id + ) + end + + it 'returns date and id from single milestone' do + epic = create(:epic) + milestone1 = create(:milestone, start_date: Date.new(2000, 1, 1), due_date: Date.new(2000, 1, 10)) + create(:issue, epic: epic, milestone: milestone1) + + results = described_class.new(epic.id) + + expect(results).to have_attributes( + start_date: milestone1.start_date, + start_date_sourcing_milestone_id: milestone1.id, + due_date: milestone1.due_date, + due_date_sourcing_milestone_id: milestone1.id + ) + end + + it 'returns date and id from milestone without date' do + epic = create(:epic) + milestone1 = create(:milestone, start_date: Date.new(2000, 1, 1)) + create(:issue, epic: epic, milestone: milestone1) + + results = described_class.new(epic.id) + + expect(results).to have_attributes( + start_date: milestone1.start_date, + start_date_sourcing_milestone_id: milestone1.id, + due_date: nil, + due_date_sourcing_milestone_id: nil + ) + end + + it 'handles epics without milestone' do + epic = create(:epic) + + results = described_class.new(epic.id) + + expect(results).to have_attributes( + start_date: nil, + start_date_sourcing_milestone_id: nil, + due_date: nil, + due_date_sourcing_milestone_id: nil + ) + end + end +end diff --git a/ee/spec/fixtures/api/schemas/entities/epic.json b/ee/spec/fixtures/api/schemas/entities/epic.json index 8d4615816150ce7a3c20973383f350250d790426..6ab2d6ae653fa958c9b873d6512c79da0a17af7d 100644 --- a/ee/spec/fixtures/api/schemas/entities/epic.json +++ b/ee/spec/fixtures/api/schemas/entities/epic.json @@ -12,7 +12,14 @@ "milestone_id": { "type": ["string", "null"] }, "state": { "type": "string" }, "start_date": { "type": ["date", "null"] }, + "start_date_fixed": { "type": ["date", "null"] }, + "start_date_is_fixed": { "type": "boolean" }, + "start_date_from_milestones": { "type": ["date", "null"] }, "end_date": { "type": ["date", "null"] }, + "due_date": { "type": ["date", "null"] }, + "due_date_fixed": { "type": ["date", "null"] }, + "due_date_from_milestones": { "type": ["date", "null"] }, + "due_date_is_fixed": { "type": "boolean" }, "updated_by_id": { "type": ["string", "null"] }, "created_at": { "type": "string" }, "updated_at": { "type": "string" }, diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/epic.json b/ee/spec/fixtures/api/schemas/public_api/v4/epic.json index 873f5c04ef4f249bbd8c46648750bc3a54005f52..a171a7f658339bd18863f85c60e7359d8138cc31 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/epic.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/epic.json @@ -24,8 +24,15 @@ "type": "string" } }, - "start_date": { "type": ["string", "null"] }, - "end_date": { "type": ["string", "null"] }, + "start_date": { "type": ["date", "null"] }, + "start_date_fixed": { "type": ["date", "null"] }, + "start_date_from_milestones": { "type": ["date", "null"] }, + "start_date_is_fixed": { "type": "boolean" }, + "end_date": { "type": ["date", "null"] }, + "due_date": { "type": ["date", "null"] }, + "due_date_fixed": { "type": ["date", "null"] }, + "due_date_from_milestones": { "type": ["date", "null"] }, + "due_date_is_fixed": { "type": "boolean" }, "created_at": { "type": ["string", "null"] }, "updated_at": { "type": ["string", "null"] } }, diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/epic_issue_link.json b/ee/spec/fixtures/api/schemas/public_api/v4/epic_issue_link.json index 74b102f95594a0134e81bf9b5e6d3deea5ce75db..63c89cc791c97c324696589640d76cacead9056d 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/epic_issue_link.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/epic_issue_link.json @@ -18,8 +18,15 @@ "group_id": { "type": "integer" }, "description": { "type": ["string", "null"] }, "author": { "type": ["object", "null"] }, - "start_date": { "type": ["string", "null"] }, - "end_date": { "type": ["string", "null"] }, + "start_date": { "type": ["date", "null"] }, + "start_date_fixed": { "type": ["date", "null"] }, + "start_date_from_milestones": { "type": ["date", "null"] }, + "start_date_is_fixed": { "type": "boolean" }, + "end_date": { "type": ["date", "null"] }, + "due_date": { "type": ["date", "null"] }, + "due_date_fixed": { "type": ["date", "null"] }, + "due_date_from_milestones": { "type": ["date", "null"] }, + "due_date_is_fixed": { "type": "boolean" }, "created_at": { "type": ["string", "null"] }, "updated_at": { "type": ["string", "null"] }, "labels": { diff --git a/ee/spec/helpers/epics_helper_spec.rb b/ee/spec/helpers/epics_helper_spec.rb index 45c57dcc91cc62d10faf5521daf159e357052c3d..02e5394295393388f13d22d58e7e686faffb3476 100644 --- a/ee/spec/helpers/epics_helper_spec.rb +++ b/ee/spec/helpers/epics_helper_spec.rb @@ -4,18 +4,21 @@ include ApplicationHelper describe '#epic_show_app_data' do - it 'returns the correct json' do - user = create(:user) - @epic = create(:epic, author: user) + let(:user) { create(:user) } + let!(:epic) { create(:epic, author: user) } + before do allow(helper).to receive(:current_user).and_return(user) + stub_licensed_features(epics: true) + end - data = helper.epic_show_app_data(@epic, initial: {}, author_icon: 'icon_path') + it 'returns the correct json' do + data = helper.epic_show_app_data(epic, initial: {}, author_icon: 'icon_path') meta_data = JSON.parse(data[:meta]) expected_keys = %i(initial meta namespace labels_path toggle_subscription_path labels_web_url epics_web_url) expect(data.keys).to match_array(expected_keys) - expect(meta_data.keys).to match_array(%w[created author start_date end_date epic_id todo_exists todo_path]) + expect(meta_data.keys).to match_array(%w[created author start_date end_date due_date epic_id todo_exists todo_path]) expect(meta_data['author']).to eq({ 'name' => user.name, 'url' => "/#{user.username}", @@ -23,6 +26,40 @@ 'src' => 'icon_path' }) end + + context 'when user has edit permission' do + let(:milestone) { create(:milestone, title: 'make me a sandwich') } + + let!(:epic) do + create( + :epic, + author: user, + start_date_sourcing_milestone: milestone, + start_date: Date.new(2000, 1, 1), + due_date_sourcing_milestone: milestone, + due_date: Date.new(2000, 1, 2) + ) + end + + before do + epic.group.add_developer(user) + end + + it 'returns extra date fields if user can edit' do + data = helper.epic_show_app_data(epic, initial: {}, author_icon: 'icon_path') + meta_data = JSON.parse(data[:meta]) + + expect(meta_data.keys).to match_array(%w[ + created author epic_id todo_exists todo_path + start_date start_date_fixed start_date_is_fixed start_date_from_milestones start_date_sourcing_milestone_title + end_date due_date due_date_fixed due_date_is_fixed due_date_from_milestones due_date_sourcing_milestone_title + ]) + expect(meta_data['start_date']).to eq('2000-01-01') + expect(meta_data['start_date_sourcing_milestone_title']).to eq(milestone.title) + expect(meta_data['due_date']).to eq('2000-01-02') + expect(meta_data['due_date_sourcing_milestone_title']).to eq(milestone.title) + end + end end describe '#epic_endpoint_query_params' do diff --git a/ee/spec/migrations/update_epic_dates_from_milestones_spec.rb b/ee/spec/migrations/update_epic_dates_from_milestones_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..42dd8e6b58fdbb7b3c21d451002ad57221d164b2 --- /dev/null +++ b/ee/spec/migrations/update_epic_dates_from_milestones_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('ee', 'db', 'post_migrate', '20180713171825_update_epic_dates_from_milestones.rb') + +describe UpdateEpicDatesFromMilestones, :migration do + let(:migration) { described_class.new } + let(:users) { table(:users) } + let(:namespaces) { table(:namespaces) } + let(:epics) { table(:epics) } + let(:projects) { table(:projects) } + let(:issues) { table(:issues) } + let(:epic_issues) { table(:epic_issues) } + let(:milestones) { table(:milestones) } + + describe '#up' do + before do + user = users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') + namespaces.create(id: 1, name: 'gitlab-org', path: 'gitlab-org') + projects.create!(id: 1, namespace_id: 1, name: 'gitlab1', path: 'gitlab1') + + epics.create!(id: 1, iid: 1, group_id: 1, title: 'epic with start and due dates', title_html: '', author_id: user.id) + epics.create!(id: 2, iid: 2, group_id: 1, title: 'epic with only due date', title_html: '', author_id: user.id) + epics.create!(id: 3, iid: 3, group_id: 1, title: 'epic without milestone', title_html: '', author_id: user.id) + + milestones.create!( + id: 1, + iid: 1, + project_id: 1, + title: 'milestone-1', + start_date: Date.new(2000, 1, 1), + due_date: Date.new(2000, 1, 10) + ) + milestones.create!( + id: 2, + iid: 2, + project_id: 1, + title: 'milestone-2', + due_date: Date.new(2000, 1, 30) + ) + + issues.create!(id: 1, iid: 1, project_id: 1, milestone_id: 1, title: 'issue-1') + issues.create!(id: 2, iid: 2, project_id: 1, milestone_id: 2, title: 'issue-2') + issues.create!(id: 3, iid: 3, project_id: 1, milestone_id: 2, title: 'issue-2') + + epic_issues.create!(epic_id: 1, issue_id: 1) + epic_issues.create!(epic_id: 1, issue_id: 2) + epic_issues.create!(epic_id: 2, issue_id: 3) + end + + it 'updates dates milestone ids' do + migration.up + + expect(Epic.find(1)).to have_attributes( + start_date: Date.new(2000, 1, 1), + start_date_sourcing_milestone_id: 1, + due_date: Date.new(2000, 1, 30), + due_date_sourcing_milestone_id: 2 + ) + expect(Epic.find(2)).to have_attributes( + start_date: nil, + start_date_sourcing_milestone_id: nil, + due_date: Date.new(2000, 1, 30), + due_date_sourcing_milestone_id: 2 + ) + expect(Epic.find(3)).to have_attributes( + start_date: nil, + start_date_sourcing_milestone_id: nil, + due_date: nil, + due_date_sourcing_milestone_id: nil + ) + end + end +end diff --git a/ee/spec/models/epic_spec.rb b/ee/spec/models/epic_spec.rb index e33378aa1627c92d583387f96cda43dba300ef84..d7fc5bd2c96aa7bbf52802a4b953578e6ecf5808 100644 --- a/ee/spec/models/epic_spec.rb +++ b/ee/spec/models/epic_spec.rb @@ -7,6 +7,7 @@ it { is_expected.to belong_to(:author).class_name('User') } it { is_expected.to belong_to(:assignee).class_name('User') } it { is_expected.to belong_to(:group) } + it { is_expected.to have_many(:epic_issues) } end describe 'validations' do @@ -84,7 +85,299 @@ end end - describe '#issues' do + describe '#start_date' do + let(:date) { Date.new(2000, 1, 1) } + + context 'is set' do + subject { build(:epic, :use_fixed_dates, start_date: date) } + + it 'returns as is' do + expect(subject.start_date).to eq(date) + end + end + end + + describe '#start_date_from_milestones' do + context 'fixed date' do + it 'returns start date from start date sourcing milestone' do + subject = create(:epic, :use_fixed_dates) + milestone = create(:milestone, :with_dates) + subject.start_date_sourcing_milestone = milestone + + expect(subject.start_date_from_milestones).to eq(milestone.start_date) + end + end + + context 'milestone date' do + it 'returns start_date' do + subject = create(:epic, start_date: Date.new(2017, 3, 4)) + + expect(subject.start_date_from_milestones).to eq(subject.start_date) + end + end + end + + describe '#due_date_from_milestones' do + context 'fixed date' do + it 'returns due date from due date sourcing milestone' do + subject = create(:epic, :use_fixed_dates) + milestone = create(:milestone, :with_dates) + subject.due_date_sourcing_milestone = milestone + + expect(subject.due_date_from_milestones).to eq(milestone.due_date) + end + end + + context 'milestone date' do + it 'returns due_date' do + subject = create(:epic, due_date: Date.new(2017, 3, 4)) + + expect(subject.due_date_from_milestones).to eq(subject.due_date) + end + end + end + + describe '#update_start_and_due_dates' do + context 'fixed date is set' do + subject { create(:epic, :use_fixed_dates, start_date: nil, end_date: nil) } + + it 'updates to fixed date' do + subject.update_start_and_due_dates + + expect(subject.start_date).to eq(subject.start_date_fixed) + expect(subject.due_date).to eq(subject.due_date_fixed) + end + end + + context 'fixed date is not set' do + subject { create(:epic, start_date: nil, end_date: nil) } + + let(:milestone1) do + create( + :milestone, + start_date: Date.new(2000, 1, 1), + due_date: Date.new(2000, 1, 10) + ) + end + let(:milestone2) do + create( + :milestone, + start_date: Date.new(2000, 1, 3), + due_date: Date.new(2000, 1, 20) + ) + end + + context 'multiple milestones' do + before do + epic_issue1 = create(:epic_issue, epic: subject) + epic_issue1.issue.update(milestone: milestone1) + epic_issue2 = create(:epic_issue, epic: subject) + epic_issue2.issue.update(milestone: milestone2) + end + + context 'complete start and due dates' do + it 'updates to milestone dates' do + subject.update_start_and_due_dates + + expect(subject.start_date).to eq(milestone1.start_date) + expect(subject.due_date).to eq(milestone2.due_date) + end + end + + context 'without due date' do + let(:milestone1) do + create( + :milestone, + start_date: Date.new(2000, 1, 1), + due_date: nil + ) + end + let(:milestone2) do + create( + :milestone, + start_date: Date.new(2000, 1, 3), + due_date: nil + ) + end + + it 'updates to milestone dates' do + subject.update_start_and_due_dates + + expect(subject.start_date).to eq(milestone1.start_date) + expect(subject.due_date).to eq(nil) + end + end + + context 'without any dates' do + let(:milestone1) do + create( + :milestone, + start_date: nil, + due_date: nil + ) + end + let(:milestone2) do + create( + :milestone, + start_date: nil, + due_date: nil + ) + end + + it 'updates to milestone dates' do + subject.update_start_and_due_dates + + expect(subject.start_date).to eq(nil) + expect(subject.due_date).to eq(nil) + end + end + end + + context 'without milestone' do + before do + create(:epic_issue, epic: subject) + end + + it 'updates to milestone dates' do + subject.update_start_and_due_dates + + expect(subject.start_date).to eq(nil) + expect(subject.start_date_sourcing_milestone_id).to eq(nil) + expect(subject.due_date).to eq(nil) + expect(subject.due_date_sourcing_milestone_id).to eq(nil) + end + end + + context 'single milestone' do + before do + epic_issue1 = create(:epic_issue, epic: subject) + epic_issue1.issue.update(milestone: milestone1) + end + + context 'complete start and due dates' do + it 'updates to milestone dates' do + subject.update_start_and_due_dates + + expect(subject.start_date).to eq(milestone1.start_date) + expect(subject.due_date).to eq(milestone1.due_date) + end + end + + context 'without due date' do + let(:milestone1) do + create( + :milestone, + start_date: Date.new(2000, 1, 1), + due_date: nil + ) + end + + it 'updates to milestone dates' do + subject.update_start_and_due_dates + + expect(subject.start_date).to eq(milestone1.start_date) + expect(subject.due_date).to eq(nil) + end + end + + context 'without any dates' do + let(:milestone1) do + create( + :milestone, + start_date: nil, + due_date: nil + ) + end + + it 'updates to milestone dates' do + subject.update_start_and_due_dates + + expect(subject.start_date).to eq(nil) + expect(subject.due_date).to eq(nil) + end + end + end + end + end + + describe '.update_start_and_due_dates' do + def link_epic_to_milestone(epic, milestone) + create(:issue, epic: epic, milestone: milestone) + end + + it 'updates in bulk' do + milestone1 = create(:milestone, start_date: Date.new(2000, 1, 1), due_date: Date.new(2000, 1, 10)) + milestone2 = create(:milestone, due_date: Date.new(2000, 1, 30)) + + epics = [ + create(:epic), + create(:epic), + create(:epic, :use_fixed_dates) + ] + old_attributes = epics.map(&:attributes) + + link_epic_to_milestone(epics[0], milestone1) + link_epic_to_milestone(epics[0], milestone2) + link_epic_to_milestone(epics[1], milestone2) + link_epic_to_milestone(epics[2], milestone1) + link_epic_to_milestone(epics[2], milestone2) + + described_class.update_start_and_due_dates(described_class.where(id: epics.map(&:id))) + + epics.each(&:reload) + + expect(epics[0].start_date).to eq(milestone1.start_date) + expect(epics[0].start_date_sourcing_milestone).to eq(milestone1) + expect(epics[0].due_date).to eq(milestone2.due_date) + expect(epics[0].due_date_sourcing_milestone).to eq(milestone2) + + expect(epics[1].start_date).to eq(nil) + expect(epics[1].start_date_sourcing_milestone).to eq(nil) + expect(epics[1].due_date).to eq(milestone2.due_date) + expect(epics[1].due_date_sourcing_milestone).to eq(milestone2) + + expect(epics[2].start_date).to eq(old_attributes[2]['start_date']) + expect(epics[2].start_date_sourcing_milestone).to eq(milestone1) + expect(epics[2].due_date).to eq(old_attributes[2]['end_date']) + expect(epics[2].due_date_sourcing_milestone).to eq(milestone2) + end + + context 'query count check' do + let(:milestone) { create(:milestone, start_date: Date.new(2000, 1, 1), due_date: Date.new(2000, 1, 10)) } + let!(:epics) { [create(:epic)] } + + def setup_control_group + link_epic_to_milestone(epics[0], milestone) + + ActiveRecord::QueryRecorder.new do + described_class.update_start_and_due_dates(described_class.where(id: epics.map(&:id))) + end.count + end + + it 'does not increase query count when adding epics without milestones' do + control_count = setup_control_group + + epics << create(:epic) + + expect do + described_class.update_start_and_due_dates(described_class.where(id: epics.map(&:id))) + end.not_to exceed_query_limit(control_count) + end + + it 'does not increase query count when adding epics belongs to same milestones' do + control_count = setup_control_group + + epics << create(:epic) + link_epic_to_milestone(epics[1], milestone) + + expect do + described_class.update_start_and_due_dates(described_class.where(id: epics.map(&:id))) + end.not_to exceed_query_limit(control_count) + end + end + end + + describe '#issues_readable_by' do let(:user) { create(:user) } let(:group) { create(:group, :private) } let(:project) { create(:project, group: group) } @@ -101,7 +394,7 @@ ] end - let(:result) { epic.issues(user) } + let(:result) { epic.issues_readable_by(user) } it 'returns all issues if a user has access to them' do group.add_developer(user) diff --git a/ee/spec/requests/api/epics_spec.rb b/ee/spec/requests/api/epics_spec.rb index bdd99dac134b260c01886be341581dd710cbb33f..7dcbca40a971ec73980a61812e63eabfd863dd2a 100644 --- a/ee/spec/requests/api/epics_spec.rb +++ b/ee/spec/requests/api/epics_spec.rb @@ -40,6 +40,32 @@ end end + shared_examples 'can admin epics' do + let(:extra_date_fields) { %w[start_date_is_fixed start_date_fixed due_date_is_fixed due_date_fixed] } + + context 'when permission is absent' do + RSpec::Matchers.define_negated_matcher :exclude, :include + + it 'returns epic with extra date fields' do + get api(url, user), params + + expect(Array.wrap(JSON.parse(response.body))).to all(exclude(*extra_date_fields)) + end + end + + context 'when permission is present' do + before do + group.add_maintainer(user) + end + + it 'returns epic with extra date fields' do + get api(url, user), params + + expect(Array.wrap(JSON.parse(response.body))).to all(include(*extra_date_fields)) + end + end + end + describe 'GET /groups/:id/epics' do let(:url) { "/groups/#{group.path}/epics" } @@ -138,6 +164,8 @@ def expect_array_response(expected) expect_array_response([epic2.id]) end + + it_behaves_like 'can admin epics' end end @@ -149,17 +177,21 @@ def expect_array_response(expected) context 'when the request is correct' do before do stub_licensed_features(epics: true) - - get api(url, user) end it 'returns 200 status' do + get api(url, user) + expect(response).to have_gitlab_http_status(200) end it 'matches the response schema' do + get api(url, user) + expect(response).to match_response_schema('public_api/v4/epic', dir: 'ee') end + + it_behaves_like 'can admin epics' end end @@ -206,13 +238,26 @@ def expect_array_response(expected) expect(epic.description).to eq('epic description') expect(epic.labels.first.title).to eq('label1') end + + context 'when deprecated start_date and end_date params are present' do + let(:start_date) { Date.new(2001, 1, 1) } + let(:due_date) { Date.new(2001, 1, 2) } + let(:params) { { title: 'new epic', start_date: start_date, end_date: due_date } } + + it 'updates start_date_fixed and due_date_fixed' do + result = Epic.last + + expect(result.start_date_fixed).to eq(start_date) + expect(result.due_date_fixed).to eq(due_date) + end + end end end end describe 'PUT /groups/:id/epics/:epic_iid' do let(:url) { "/groups/#{group.path}/epics/#{epic.iid}" } - let(:params) { { title: 'new title', description: 'new description', labels: 'label2' } } + let(:params) { { title: 'new title', description: 'new description', labels: 'label2', start_date_fixed: "2018-07-17", start_date_is_fixed: true } } it_behaves_like 'error requests' @@ -260,6 +305,23 @@ def expect_array_response(expected) expect(result.title).to eq('new title') expect(result.description).to eq('new description') expect(result.labels.first.title).to eq('label2') + expect(result.start_date).to eq(Date.new(2018, 7, 17)) + expect(result.start_date_fixed).to eq(Date.new(2018, 7, 17)) + expect(result.start_date_is_fixed).to eq(true) + end + + context 'when deprecated start_date and end_date params are present' do + let(:epic) { create(:epic, :use_fixed_dates, group: group) } + let(:new_start_date) { epic.start_date + 1.day } + let(:new_due_date) { epic.end_date + 1.day } + let!(:params) { { start_date: new_start_date, end_date: new_due_date } } + + it 'updates start_date_fixed and due_date_fixed' do + result = epic.reload + + expect(result.start_date_fixed).to eq(new_start_date) + expect(result.due_date_fixed).to eq(new_due_date) + end end end end diff --git a/ee/spec/services/ee/issues/update_service_spec.rb b/ee/spec/services/ee/issues/update_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..90f82d5b57ff7a6810a776f90898b1ae703d63bc --- /dev/null +++ b/ee/spec/services/ee/issues/update_service_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Issues::UpdateService do + let(:issue) { create(:issue) } + let(:user) { issue.author } + let(:project) { issue.project } + + describe 'execute' do + def update_issue(opts) + described_class.new(project, user, opts).execute(issue) + end + + context 'refresh epic dates' do + let(:epic) { create(:epic) } + let(:issue) { create(:issue, epic: epic) } + + context 'updating milestone' do + let(:milestone) { create(:milestone) } + + it 'calls epic#update_start_and_due_dates' do + expect(epic).to receive(:update_start_and_due_dates).twice + + update_issue(milestone: milestone) + update_issue(milestone_id: nil) + end + end + + context 'updating other fields' do + it 'does not call epic#update_start_and_due_dates' do + expect(epic).not_to receive(:update_start_and_due_dates) + update_issue(title: 'foo') + end + end + end + end +end diff --git a/ee/spec/services/epic_issues/create_service_spec.rb b/ee/spec/services/epic_issues/create_service_spec.rb index 3db29ab7034005c331e325b468ced3812db1157c..8034a04045e4d6d8412df1efad64afe4f7bc1ba2 100644 --- a/ee/spec/services/epic_issues/create_service_spec.rb +++ b/ee/spec/services/epic_issues/create_service_spec.rb @@ -268,6 +268,14 @@ def assign_issue(references) include_examples 'returns an error' end + + context 'refresh epic dates' do + it 'calls epic#update_start_and_due_dates' do + expect(epic).to receive(:update_start_and_due_dates) + + assign_issue([valid_reference]) + end + end end end end diff --git a/ee/spec/services/epic_issues/destroy_service_spec.rb b/ee/spec/services/epic_issues/destroy_service_spec.rb index 02e0594403cb6d9058b51f0c73d07eaadc9e3c42..91751fc4c0295b76e667c0ec08f0c2256d75b373 100644 --- a/ee/spec/services/epic_issues/destroy_service_spec.rb +++ b/ee/spec/services/epic_issues/destroy_service_spec.rb @@ -75,6 +75,14 @@ is_expected.to eq(message: 'No Issue Link found', status: :error, http_status: 404) end end + + context 'refresh epic dates' do + it 'calls epic#update_start_and_due_dates' do + expect(epic).to receive(:update_start_and_due_dates) + + subject + end + end end end end diff --git a/ee/spec/services/epics/update_service_spec.rb b/ee/spec/services/epics/update_service_spec.rb index 58a9525d502e93c5321f31033fe82d5c51701405..052e3c52c9dbfcabf5253e39be55b62386134442 100644 --- a/ee/spec/services/epics/update_service_spec.rb +++ b/ee/spec/services/epics/update_service_spec.rb @@ -18,8 +18,10 @@ def update_epic(opts) { title: 'New title', description: 'New description', - start_date: '2017-01-09', - end_date: '2017-10-21' + start_date_fixed: '2017-01-09', + start_date_is_fixed: true, + due_date_fixed: '2017-10-21', + due_date_is_fixed: true } end @@ -27,10 +29,11 @@ def update_epic(opts) update_epic(opts) expect(epic).to be_valid - expect(epic.title).to eq(opts[:title]) - expect(epic.description).to eq(opts[:description]) - expect(epic.start_date).to eq(Date.strptime(opts[:start_date])) - expect(epic.end_date).to eq(Date.strptime(opts[:end_date])) + expect(epic).to have_attributes(opts.except(:due_date_fixed, :start_date_fixed)) + expect(epic).to have_attributes( + start_date_fixed: Date.strptime(opts[:start_date_fixed]), + due_date_fixed: Date.strptime(opts[:due_date_fixed]) + ) end it 'updates the last_edited_at value' do @@ -115,5 +118,32 @@ def update_epic(opts) end end end + + context 'filter out start_date and end_date' do + it 'ignores start_date and end_date' do + expect { update_epic(start_date: Date.today, end_date: Date.today) }.not_to change { Note.count } + + expect(epic).to be_valid + expect(epic).to have_attributes(start_date: nil, due_date: nil) + end + end + + context 'refresh epic dates' do + context 'date fields are updated' do + it 'calls epic#update_start_and_due_dates' do + expect(epic).to receive(:update_start_and_due_dates) + + update_epic(start_date_is_fixed: true, start_date_fixed: Date.today) + end + end + + context 'date fields are not updated' do + it 'does not call epic#update_start_and_due_dates' do + expect(epic).not_to receive(:update_start_and_due_dates) + + update_epic(title: 'foo') + end + end + end end end diff --git a/ee/spec/services/milestones/update_service_spec.rb b/ee/spec/services/milestones/update_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..bdf13abc89b6ecf15c7e8986b29a13093034f3e9 --- /dev/null +++ b/ee/spec/services/milestones/update_service_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Milestones::UpdateService do + describe '#execute' do + context 'refresh related epic dates' do + it 'updates milestone sourced dates' do + project = create(:project) + user = build(:user) + milestone = create(:milestone, project: project) + epic = create(:epic) + create(:issue, milestone: milestone, epic: epic) + due_date = 3.days.from_now.to_date + + described_class.new(project, user, { due_date: due_date }).execute(milestone) + + expect(epic.reload).to have_attributes( + start_date: nil, + start_date_sourcing_milestone: nil, + due_date: due_date, + due_date_sourcing_milestone: milestone + ) + end + end + end +end diff --git a/spec/factories/milestones.rb b/spec/factories/milestones.rb index f95632e7187f5b23ee1c92a869a5192c33c44759..019e44202122dd23a8b7a9dc02d6754373b1a56b 100644 --- a/spec/factories/milestones.rb +++ b/spec/factories/milestones.rb @@ -18,6 +18,11 @@ state "closed" end + trait :with_dates do + start_date { Date.new(2000, 1, 1) } + due_date { Date.new(2000, 1, 30) } + end + after(:build, :stub) do |milestone, evaluator| if evaluator.group milestone.group = evaluator.group diff --git a/spec/services/milestones/update_service_spec.rb b/spec/services/milestones/update_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..3b91442c0ba084e71b3d7f2376e30daabcec2a5c --- /dev/null +++ b/spec/services/milestones/update_service_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Milestones::UpdateService do + let(:project) { create(:project) } + let(:user) { build(:user) } + let(:milestone) { create(:milestone, project: project) } + + describe '#execute' do + context "valid params" do + let(:inner_service) { double(:service) } + + before do + project.add_maintainer(user) + end + + subject { described_class.new(project, user, { title: 'new_title' }).execute(milestone) } + + it { expect(subject).to be_valid } + it { expect(subject.title).to eq('new_title') } + + context 'state_event is activate' do + it 'calls ReopenService' do + expect(Milestones::ReopenService).to receive(:new).with(project, user, {}).and_return(inner_service) + expect(inner_service).to receive(:execute).with(milestone) + + described_class.new(project, user, { state_event: 'activate' }).execute(milestone) + end + end + + context 'state_event is close' do + it 'calls ReopenService' do + expect(Milestones::CloseService).to receive(:new).with(project, user, {}).and_return(inner_service) + expect(inner_service).to receive(:execute).with(milestone) + + described_class.new(project, user, { state_event: 'close' }).execute(milestone) + end + end + end + end +end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 0fd37c95e429380a7018c7fbbae778cf659d8294..72c5662e866afbdddad92edb07d1a362d4deb024 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -145,7 +145,11 @@ let(:note_text) { %(HELLO\n/close\n/assign @#{user.username}\nWORLD) } it 'saves the note and does not alter the note text' do - expect_any_instance_of(Issues::UpdateService).to receive(:execute).and_call_original + service = instance_double('Issues::UpdateService', :service) + + allow(Issues::UpdateService).to receive(:new).and_return(service) + + expect(service).to receive(:execute) note = described_class.new(project, user, opts.merge(note: note_text)).execute diff --git a/spec/support/shared_examples/services/boards/issues_move_service.rb b/spec/support/shared_examples/services/boards/issues_move_service.rb index 737863ea41126cddd6881b0475449ae492896dbf..17d1fda9410454814af238adcd4ce1c326c1f8e2 100644 --- a/spec/support/shared_examples/services/boards/issues_move_service.rb +++ b/spec/support/shared_examples/services/boards/issues_move_service.rb @@ -4,7 +4,11 @@ let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list2.id } } it 'delegates the label changes to Issues::UpdateService' do - expect_any_instance_of(Issues::UpdateService).to receive(:execute).with(issue).once + service = instance_double('Issues::UpdateService', :service) + + allow(Issues::UpdateService).to receive(:new).and_return(service) + + expect(service).to receive(:execute).with(issue).once described_class.new(parent, user, params).execute(issue) end