Commit d8acf44c authored by Kerri Miller's avatar Kerri Miller

Refactor tracking of title, desc, and draft status changes

When I initially added the tracking logic here, I hadn't been able to
track down where we were triggering additional update events, which was
resetting ActiveModel::Dirty#changes (etc) pushing me to cobble together
holding state of incoming changes. This was a fine approach, but it
avoided tackling the complexity of this service head-on. In returning to
the code today, I see now how #mark_as_unchecked is triggering an update
on state change (thanks to the state machine behind that..) and can
sequence the code more appropriately and, critically, more flexibly for
deliverables I have in this release.
parent e4206fbd
...@@ -25,12 +25,14 @@ module MergeRequests ...@@ -25,12 +25,14 @@ module MergeRequests
update_task_event(merge_request) || update(merge_request) update_task_event(merge_request) || update(merge_request)
end end
# rubocop:disable Metrics/AbcSize
def handle_changes(merge_request, options) def handle_changes(merge_request, options)
old_associations = options.fetch(:old_associations, {}) old_associations = options.fetch(:old_associations, {})
old_labels = old_associations.fetch(:labels, []) old_labels = old_associations.fetch(:labels, [])
old_mentioned_users = old_associations.fetch(:mentioned_users, []) old_mentioned_users = old_associations.fetch(:mentioned_users, [])
old_assignees = old_associations.fetch(:assignees, []) old_assignees = old_associations.fetch(:assignees, [])
old_reviewers = old_associations.fetch(:reviewers, []) old_reviewers = old_associations.fetch(:reviewers, [])
changed_fields = merge_request.previous_changes.keys
if has_changes?(merge_request, old_labels: old_labels, old_assignees: old_assignees, old_reviewers: old_reviewers) if has_changes?(merge_request, old_labels: old_labels, old_assignees: old_assignees, old_reviewers: old_reviewers)
todo_service.resolve_todos_for_target(merge_request, current_user) todo_service.resolve_todos_for_target(merge_request, current_user)
...@@ -52,8 +54,11 @@ module MergeRequests ...@@ -52,8 +54,11 @@ module MergeRequests
end end
handle_assignees_change(merge_request, old_assignees) if merge_request.assignees != old_assignees handle_assignees_change(merge_request, old_assignees) if merge_request.assignees != old_assignees
handle_reviewers_change(merge_request, old_reviewers) if merge_request.reviewers != old_reviewers handle_reviewers_change(merge_request, old_reviewers) if merge_request.reviewers != old_reviewers
handle_milestone_change(merge_request)
handle_draft_status_change(merge_request, changed_fields)
track_title_and_desc_edits(merge_request, changed_fields)
if merge_request.previous_changes.include?('target_branch') || if merge_request.previous_changes.include?('target_branch') ||
merge_request.previous_changes.include?('source_branch') merge_request.previous_changes.include?('source_branch')
...@@ -81,6 +86,7 @@ module MergeRequests ...@@ -81,6 +86,7 @@ module MergeRequests
) )
end end
end end
# rubocop:enable Metrics/AbcSize
def handle_task_changes(merge_request) def handle_task_changes(merge_request)
todo_service.resolve_todos_for_target(merge_request, current_user) todo_service.resolve_todos_for_target(merge_request, current_user)
...@@ -95,50 +101,46 @@ module MergeRequests ...@@ -95,50 +101,46 @@ module MergeRequests
MergeRequests::CloseService MergeRequests::CloseService
end end
def before_update(issuable, skip_spam_check: false)
return unless issuable.changed?
@issuable_changes = issuable.changes
end
def after_update(issuable) def after_update(issuable)
issuable.cache_merge_request_closes_issues!(current_user) issuable.cache_merge_request_closes_issues!(current_user)
end
private
return unless @issuable_changes attr_reader :target_branch_was_deleted
%w(title description).each do |action| def track_title_and_desc_edits(merge_request, changed_fields)
next unless @issuable_changes.key?(action) tracked_fields = %w(title description)
return unless changed_fields.any? { |i| tracked_fields.include? i }
tracked_fields.each do |action|
next unless changed_fields.include?(action)
# Track edits to title or description
#
merge_request_activity_counter merge_request_activity_counter
.public_send("track_#{action}_edit_action".to_sym, user: current_user) # rubocop:disable GitlabSecurity/PublicSend .public_send("track_#{action}_edit_action".to_sym, user: current_user) # rubocop:disable GitlabSecurity/PublicSend
# Track changes to Draft/WIP status
#
if action == "title"
old_title, new_title = @issuable_changes["title"]
old_title_wip = MergeRequest.work_in_progress?(old_title)
new_title_wip = MergeRequest.work_in_progress?(new_title)
if !old_title_wip && new_title_wip
# Marked as Draft/WIP
#
merge_request_activity_counter
.track_marked_as_draft_action(user: current_user)
elsif old_title_wip && !new_title_wip
# Unmarked as Draft/WIP
#
merge_request_activity_counter
.track_unmarked_as_draft_action(user: current_user)
end
end
end end
end end
private def handle_draft_status_change(merge_request, changed_fields)
return unless changed_fields.include?("title")
attr_reader :target_branch_was_deleted old_title, new_title = merge_request.previous_changes["title"]
old_title_wip = MergeRequest.work_in_progress?(old_title)
new_title_wip = MergeRequest.work_in_progress?(new_title)
if !old_title_wip && new_title_wip
# Marked as Draft/WIP
#
merge_request_activity_counter
.track_marked_as_draft_action(user: current_user)
elsif old_title_wip && !new_title_wip
# Unmarked as Draft/WIP
#
merge_request_activity_counter
.track_unmarked_as_draft_action(user: current_user)
end
end
def handle_milestone_change(merge_request) def handle_milestone_change(merge_request)
return if skip_milestone_email return if skip_milestone_email
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment