Commit 093f5a50 authored by Mark Chao's avatar Mark Chao

Merge branch 'caw-use-base-container-service' into 'master'

Migrate issue service subclasses to BaseProjectServices [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!59182
parents 81a639eb c5690b79
......@@ -2094,6 +2094,7 @@ Gitlab/NamespacedClass:
- 'app/services/auto_merge_service.rb'
- 'app/services/base_container_service.rb'
- 'app/services/base_count_service.rb'
- 'app/services/base_project_service.rb'
- 'app/services/base_renderer.rb'
- 'app/services/base_service.rb'
- 'app/services/bulk_create_integration_service.rb'
......
......@@ -61,7 +61,7 @@ module IssuableActions
end
def destroy
Issuable::DestroyService.new(issuable.project, current_user).execute(issuable)
Issuable::DestroyService.new(project: issuable.project, current_user: current_user).execute(issuable)
name = issuable.human_class_name
flash[:notice] = "The #{name} was successfully deleted."
......
......@@ -113,7 +113,7 @@ class Projects::IssuesController < Projects::ApplicationController
discussion_to_resolve: params[:discussion_to_resolve],
confidential: !!Gitlab::Utils.to_boolean(issue_params[:confidential])
)
service = ::Issues::BuildService.new(project, current_user, build_params)
service = ::Issues::BuildService.new(project: project, current_user: current_user, params: build_params)
@issue = @noteable = service.execute
......@@ -133,7 +133,7 @@ class Projects::IssuesController < Projects::ApplicationController
discussion_to_resolve: params[:discussion_to_resolve]
)
service = ::Issues::CreateService.new(project, current_user, create_params)
service = ::Issues::CreateService.new(project: project, current_user: current_user, params: create_params)
@issue = service.execute
create_vulnerability_issue_feedback(issue)
......@@ -160,7 +160,7 @@ class Projects::IssuesController < Projects::ApplicationController
new_project = Project.find(params[:move_to_project_id])
return render_404 unless issue.can_move?(current_user, new_project)
@issue = ::Issues::UpdateService.new(project, current_user, target_project: new_project).execute(issue)
@issue = ::Issues::UpdateService.new(project: project, current_user: current_user, params: { target_project: new_project }).execute(issue)
end
respond_to do |format|
......@@ -174,7 +174,7 @@ class Projects::IssuesController < Projects::ApplicationController
end
def reorder
service = ::Issues::ReorderService.new(project, current_user, reorder_params)
service = ::Issues::ReorderService.new(project: project, current_user: current_user, params: reorder_params)
if service.execute(issue)
head :ok
......@@ -185,7 +185,7 @@ class Projects::IssuesController < Projects::ApplicationController
def related_branches
@related_branches = ::Issues::RelatedBranchesService
.new(project, current_user)
.new(project: project, current_user: current_user)
.execute(issue)
.map { |branch| branch.merge(link: branch_link(branch)) }
......@@ -213,7 +213,7 @@ class Projects::IssuesController < Projects::ApplicationController
def create_merge_request
create_params = params.slice(:branch_name, :ref).merge(issue_iid: issue.iid)
create_params[:target_project_id] = params[:target_project_id]
result = ::MergeRequests::CreateFromIssueService.new(project, current_user, create_params).execute
result = ::MergeRequests::CreateFromIssueService.new(project: project, current_user: current_user, mr_params: create_params).execute
if result[:status] == :success
render json: MergeRequestCreateSerializer.new.represent(result[:merge_request])
......@@ -334,7 +334,7 @@ class Projects::IssuesController < Projects::ApplicationController
def update_service
update_params = issue_params.merge(spammable_params)
::Issues::UpdateService.new(project, current_user, update_params)
::Issues::UpdateService.new(project: project, current_user: current_user, params: update_params)
end
def finder_type
......
......@@ -19,7 +19,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
end
def create
@merge_request = ::MergeRequests::CreateService.new(project, current_user, merge_request_params).execute
@merge_request = ::MergeRequests::CreateService.new(project: project, current_user: current_user, params: merge_request_params).execute
if @merge_request.valid?
incr_count_webide_merge_request
......@@ -93,7 +93,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
# Gitaly N+1 issue: https://gitlab.com/gitlab-org/gitlab-foss/issues/58096
Gitlab::GitalyClient.allow_n_plus_1_calls do
@merge_request = ::MergeRequests::BuildService.new(project, current_user, merge_request_params.merge(diff_options: diff_options)).execute
@merge_request = ::MergeRequests::BuildService.new(project: project, current_user: current_user, params: merge_request_params.merge(diff_options: diff_options)).execute
end
end
......
......@@ -245,7 +245,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end
def update
@merge_request = ::MergeRequests::UpdateService.new(project, current_user, merge_request_update_params).execute(@merge_request)
@merge_request = ::MergeRequests::UpdateService.new(project: project, current_user: current_user, params: merge_request_update_params).execute(@merge_request)
respond_to do |format|
format.html do
......@@ -274,7 +274,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
def remove_wip
@merge_request = ::MergeRequests::UpdateService
.new(project, current_user, wip_event: 'unwip')
.new(project: project, current_user: current_user, params: { wip_event: 'unwip' })
.execute(@merge_request)
render json: serialize_widget(@merge_request)
......@@ -309,7 +309,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end
def assign_related_issues
result = ::MergeRequests::AssignIssuesService.new(project, current_user, merge_request: @merge_request).execute
result = ::MergeRequests::AssignIssuesService.new(project: project, current_user: current_user, params: { merge_request: @merge_request }).execute
case result[:count]
when 0
......@@ -421,7 +421,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
return :failed
end
merge_service = ::MergeRequests::MergeService.new(@project, current_user, merge_params)
merge_service = ::MergeRequests::MergeService.new(project: @project, current_user: current_user, params: merge_params)
unless merge_service.hooks_validation_pass?(@merge_request)
return :hook_validation_error
......
......@@ -33,9 +33,9 @@ module Mutations
def assign!(resource, users, operation_mode)
update_service_class.new(
resource.project,
current_user,
assignee_ids: assignee_ids(resource, users, operation_mode)
project: resource.project,
current_user: current_user,
params: { assignee_ids: assignee_ids(resource, users, operation_mode) }
).execute(resource)
end
......
......@@ -73,7 +73,7 @@ module Mutations
project = authorized_find!(project_path)
params = build_create_issue_params(attributes.merge(author_id: current_user.id))
issue = ::Issues::CreateService.new(project, current_user, params).execute
issue = ::Issues::CreateService.new(project: project, current_user: current_user, params: params).execute
if issue.spam?
issue.errors.add(:base, 'Spam detected.')
......
......@@ -18,7 +18,7 @@ module Mutations
target_project = resolve_project(full_path: target_project_path).sync
begin
moved_issue = ::Issues::MoveService.new(source_project, current_user).execute(issue, target_project)
moved_issue = ::Issues::MoveService.new(project: source_project, current_user: current_user).execute(issue, target_project)
rescue ::Issues::MoveService::MoveError => error
errors = error.message
end
......
......@@ -14,7 +14,7 @@ module Mutations
issue = authorized_find!(project_path: project_path, iid: iid)
project = issue.project
::Issues::UpdateService.new(project, current_user, confidential: confidential)
::Issues::UpdateService.new(project: project, current_user: current_user, params: { confidential: confidential })
.execute(issue)
{
......
......@@ -23,7 +23,7 @@ module Mutations
issue = authorized_find!(project_path: project_path, iid: iid)
project = issue.project
::Issues::UpdateService.new(project, current_user, due_date: due_date)
::Issues::UpdateService.new(project: project, current_user: current_user, params: { due_date: due_date })
.execute(issue)
{
......
......@@ -13,7 +13,7 @@ module Mutations
def resolve(project_path:, iid:, locked:)
issue = authorized_find!(project_path: project_path, iid: iid)
::Issues::UpdateService.new(issue.project, current_user, discussion_locked: locked)
::Issues::UpdateService.new(project: issue.project, current_user: current_user, params: { discussion_locked: locked })
.execute(issue)
{
......
......@@ -12,7 +12,7 @@ module Mutations
issue = authorized_find!(project_path: project_path, iid: iid)
project = issue.project
::Issues::UpdateService.new(project, current_user, severity: severity)
::Issues::UpdateService.new(project: project, current_user: current_user, params: { severity: severity })
.execute(issue)
{
......
......@@ -31,7 +31,7 @@ module Mutations
issue = authorized_find!(project_path: project_path, iid: iid)
project = issue.project
::Issues::UpdateService.new(project, current_user, args).execute(issue)
::Issues::UpdateService.new(project: project, current_user: current_user, params: args).execute(issue)
{
issue: issue,
......
......@@ -47,7 +47,7 @@ module Mutations
merge_request = authorized_find!(project_path: project_path, iid: iid)
project = merge_request.target_project
merge_params = args.compact.with_indifferent_access
merge_service = ::MergeRequests::MergeService.new(project, current_user, merge_params)
merge_service = ::MergeRequests::MergeService.new(project: project, current_user: current_user, params: merge_params)
if error = validate(merge_request, merge_service, merge_params)
return { merge_request: merge_request, errors: [error] }
......
......@@ -42,7 +42,7 @@ module Mutations
project = authorized_find!(project_path)
params = attributes.merge(author_id: current_user.id)
merge_request = ::MergeRequests::CreateService.new(project, current_user, params).execute
merge_request = ::MergeRequests::CreateService.new(project: project, current_user: current_user, params: params).execute
{
merge_request: merge_request.valid? ? merge_request : nil,
......
......@@ -15,7 +15,7 @@ module Mutations
def resolve(project_path:, iid:, user:)
merge_request = authorized_find!(project_path: project_path, iid: iid)
result = ::MergeRequests::RequestReviewService.new(merge_request.project, current_user).execute(merge_request, user)
result = ::MergeRequests::RequestReviewService.new(project: merge_request.project, current_user: current_user).execute(merge_request, user)
{
merge_request: merge_request,
......
......@@ -16,7 +16,7 @@ module Mutations
merge_request = authorized_find!(project_path: project_path, iid: iid)
project = merge_request.project
::MergeRequests::UpdateService.new(project, current_user, wip_event: wip_event(draft))
::MergeRequests::UpdateService.new(project: project, current_user: current_user, params: { wip_event: wip_event(draft) })
.execute(merge_request)
{
......
......@@ -9,14 +9,14 @@ module Mutations
[::Types::GlobalIDType[Label]],
required: true,
description: <<~DESC
The Label IDs to set. Replaces existing labels by default.
The Label IDs to set. Replaces existing labels by default.
DESC
argument :operation_mode,
Types::MutationOperationModeEnum,
required: false,
description: <<~DESC
Changes the operation mode. Defaults to REPLACE.
Changes the operation mode. Defaults to REPLACE.
DESC
def resolve(project_path:, iid:, label_ids:, operation_mode: Types::MutationOperationModeEnum.enum[:replace])
......@@ -38,7 +38,7 @@ module Mutations
:label_ids
end
::MergeRequests::UpdateService.new(project, current_user, attribute_name => label_ids)
::MergeRequests::UpdateService.new(project: project, current_user: current_user, params: { attribute_name => label_ids })
.execute(merge_request)
{
......
......@@ -9,14 +9,14 @@ module Mutations
GraphQL::BOOLEAN_TYPE,
required: true,
description: <<~DESC
Whether or not to lock the merge request.
Whether or not to lock the merge request.
DESC
def resolve(project_path:, iid:, locked:)
merge_request = authorized_find!(project_path: project_path, iid: iid)
project = merge_request.project
::MergeRequests::UpdateService.new(project, current_user, discussion_locked: locked)
::MergeRequests::UpdateService.new(project: project, current_user: current_user, params: { discussion_locked: locked })
.execute(merge_request)
{
......
......@@ -10,14 +10,14 @@ module Mutations
required: false,
loads: Types::MilestoneType,
description: <<~DESC
The milestone to assign to the merge request.
The milestone to assign to the merge request.
DESC
def resolve(project_path:, iid:, milestone: nil)
merge_request = authorized_find!(project_path: project_path, iid: iid)
project = merge_request.project
::MergeRequests::UpdateService.new(project, current_user, milestone: milestone)
::MergeRequests::UpdateService.new(project: project, current_user: current_user, params: { milestone: milestone })
.execute(merge_request)
{
......
......@@ -16,7 +16,7 @@ module Mutations
merge_request = authorized_find!(project_path: project_path, iid: iid)
project = merge_request.project
::MergeRequests::UpdateService.new(project, current_user, wip_event: wip_event(merge_request, wip))
::MergeRequests::UpdateService.new(project: project, current_user: current_user, params: { wip_event: wip_event(merge_request, wip) })
.execute(merge_request)
{
......
......@@ -29,7 +29,7 @@ module Mutations
attributes = args.compact
::MergeRequests::UpdateService
.new(merge_request.project, current_user, attributes)
.new(project: merge_request.project, current_user: current_user, params: attributes)
.execute(merge_request)
errors = errors_on_object(merge_request)
......
......@@ -1741,7 +1741,7 @@ class MergeRequest < ApplicationRecord
if project.resolve_outdated_diff_discussions?
MergeRequests::ResolvedDiscussionNotificationService
.new(project, current_user)
.new(project: project, current_user: current_user)
.execute(self)
end
end
......
......@@ -151,11 +151,12 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
def assign_to_closing_issues_link
# rubocop: disable CodeReuse/ServiceClass
issues = MergeRequests::AssignIssuesService.new(project,
current_user,
merge_request: merge_request,
closes_issues: closing_issues
).assignable_issues
issues = MergeRequests::AssignIssuesService.new(project: project,
current_user: current_user,
params: {
merge_request: merge_request,
closes_issues: closing_issues
}).assignable_issues
path = assign_related_issues_project_merge_request_path(project, merge_request)
if issues.present?
if issues.count > 1
......
# frozen_string_literal: true
# Base class, scoped by container (project or group)
# Base class, scoped by container (project or group).
#
# New or existing services which only require project as a container
# should subclass BaseProjectService.
#
# If you require a different but specific, non-polymorphic container (such
# as group), consider creating a new subclass such as BaseGroupService,
# and update the related comment at the top of the original BaseService.
class BaseContainerService
include BaseServiceUtility
......
# frozen_string_literal: true
# Base class, scoped by project
class BaseProjectService < ::BaseContainerService
attr_accessor :project
def initialize(project:, current_user: nil, params: {})
super(container: project, current_user: current_user, params: params)
@project = project
end
delegate :repository, to: :project
end
......@@ -6,9 +6,12 @@
# and existing service will use these one by one.
# After all are migrated, we can remove this class.
#
# TODO: New services should consider inheriting from
# BaseContainerService, or create new base class:
# https://gitlab.com/gitlab-org/gitlab/-/issues/216672
# New services should consider inheriting from:
#
# - BaseContainerService for services scoped by container (project or group)
# - BaseProjectService for services scoped to projects
#
# or, create a new base class and update this comment.
class BaseService
include BaseServiceUtility
......
......@@ -30,7 +30,7 @@ module Boards
end
def create_issue(params)
::Issues::CreateService.new(project, current_user, params).execute
::Issues::CreateService.new(project: project, current_user: current_user, params: params).execute
end
end
end
......
......@@ -52,7 +52,7 @@ module Boards
end
def update(issue, issue_modification_params)
::Issues::UpdateService.new(issue.project, current_user, issue_modification_params).execute(issue)
::Issues::UpdateService.new(project: issue.project, current_user: current_user, params: issue_modification_params).execute(issue)
end
def reposition_parent
......
......@@ -18,7 +18,7 @@ module Ci
AfterRequeueJobService.new(project, current_user).execute(build)
::MergeRequests::AddTodoWhenBuildFailsService
.new(project, current_user)
.new(project: project, current_user: current_user)
.close(new_build)
end
end
......
......@@ -29,7 +29,7 @@ module Ci
pipeline.reset_source_bridge!(current_user)
::MergeRequests::AddTodoWhenBuildFailsService
.new(project, current_user)
.new(project: project, current_user: current_user)
.close_all(pipeline)
Ci::ProcessPipelineService
......
......@@ -56,7 +56,7 @@ module AlertManagement
return if issue.blank? || issue.closed?
::Issues::CloseService
.new(project, User.alert_bot)
.new(project: project, current_user: User.alert_bot)
.execute(issue, system_note: false)
SystemNoteService.auto_resolve_prometheus_alert(issue, project, User.alert_bot) if issue.reset.closed?
......
......@@ -44,7 +44,7 @@ module Discussions
Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter
.track_resolve_thread_action(user: current_user)
MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(merge_request)
end
SystemNoteService.discussion_continued_in_issue(discussion, project, current_user, follow_up_issue) if follow_up_issue
......
......@@ -24,7 +24,7 @@ module DraftNotes
create_note_from_draft(draft)
draft.delete
MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(merge_request)
end
def publish_draft_notes
......@@ -41,7 +41,7 @@ module DraftNotes
set_reviewed
notification_service.async.new_review(review)
MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(merge_request)
end
def create_note_from_draft(draft)
......@@ -68,7 +68,7 @@ module DraftNotes
end
def set_reviewed
::MergeRequests::MarkReviewerReviewedService.new(project, current_user).execute(merge_request)
::MergeRequests::MarkReviewerReviewedService.new(project: project, current_user: current_user).execute(merge_request)
end
end
end
......@@ -35,7 +35,7 @@ module ErrorTracking
def close_issue(issue)
Issues::CloseService
.new(project, current_user)
.new(project: project, current_user: current_user)
.execute(issue, system_note: false)
end
......
......@@ -77,7 +77,7 @@ module Git
def merge_request_branches_for(ref_type, changes)
return [] if ref_type == :tag
MergeRequests::PushedBranchesService.new(project, current_user, changes: changes).execute
MergeRequests::PushedBranchesService.new(project: project, current_user: current_user, params: { changes: changes }).execute
end
end
end
......@@ -15,11 +15,13 @@ module IncidentManagement
def execute
issue = Issues::CreateService.new(
project,
current_user,
title: title,
description: description,
issue_type: ISSUE_TYPE
project: project,
current_user: current_user,
params: {
title: title,
description: description,
issue_type: ISSUE_TYPE
}
).execute
return error(issue.errors.full_messages.to_sentence, issue) unless issue.valid?
......
......@@ -57,7 +57,7 @@ module Issuable
items.each do |issuable|
next unless can?(current_user, :"update_#{type}", issuable)
update_class.new(issuable.issuing_parent, current_user, params).execute(issuable)
update_class.new(**update_class.constructor_container_arg(issuable.issuing_parent), current_user: current_user, params: params).execute(issuable)
end
items
......
......@@ -65,7 +65,7 @@ module Issuable
end
def close_issue
close_service = Issues::CloseService.new(old_project, current_user)
close_service = Issues::CloseService.new(project: old_project, current_user: current_user)
close_service.execute(original_entity, notifications: false, system_note: false)
end
......
# frozen_string_literal: true
module Issuable
class CommonSystemNotesService < ::BaseService
class CommonSystemNotesService < ::BaseProjectService
attr_reader :issuable
def execute(issuable, old_labels: [], old_milestone: nil, is_update: true)
......
......@@ -68,7 +68,7 @@ module Issuable
end
def create_issuable(attributes)
create_issuable_class.new(@project, @user, attributes).execute
create_issuable_class.new(project: @project, current_user: @user, params: attributes).execute
end
def email_results_to_user
......
# frozen_string_literal: true
class IssuableBaseService < BaseService
class IssuableBaseService < ::BaseProjectService
private
def self.constructor_container_arg(value)
# TODO: Dynamically determining the type of a constructor arg based on the class is an antipattern,
# but the root cause is that Epics::BaseService has some issues that inheritance may not be the
# appropriate pattern. See more details in comments at the top of Epics::BaseService#initialize.
# Follow on issue to address this:
# https://gitlab.com/gitlab-org/gitlab/-/issues/328438
{ project: value }
end
attr_accessor :params, :skip_milestone_email
def initialize(project, user = nil, params = {})
def initialize(project:, current_user: nil, params: {})
super
@skip_milestone_email = @params.delete(:skip_milestone_email)
......@@ -343,9 +353,13 @@ class IssuableBaseService < BaseService
def change_state(issuable)
case params.delete(:state_event)
when 'reopen'
reopen_service.new(project, current_user, {}).execute(issuable)
service_class = reopen_service
when 'close'
close_service.new(project, current_user, {}).execute(issuable)
service_class = close_service
end
if service_class
service_class.new(**service_class.constructor_container_arg(project), current_user: current_user).execute(issuable)
end
end
......@@ -406,7 +420,7 @@ class IssuableBaseService < BaseService
end
def create_system_notes(issuable, **options)
Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, **options)
Issuable::CommonSystemNotesService.new(project: project, current_user: current_user).execute(issuable, **options)
end
def associations_before_update(issuable)
......
......@@ -57,7 +57,7 @@ module Issues
# Skip creation of system notes for existing attributes of the issue. The system notes of the old
# issue are copied over so we don't want to end up with duplicate notes.
CreateService.new(target_project, current_user, new_params).execute(skip_system_notes: true)
CreateService.new(project: target_project, current_user: current_user, params: new_params).execute(skip_system_notes: true)
end
def queue_copy_designs
......
......@@ -8,7 +8,7 @@ module Issues
@request = params.delete(:request)
@spam_params = Spam::SpamActionService.filter_spam_params!(params, @request)
@issue = BuildService.new(project, current_user, params).execute
@issue = BuildService.new(project: project, current_user: current_user, params: params).execute
filter_resolve_discussion_params
......
......@@ -10,7 +10,7 @@ module Issues
create_issue_duplicate_note(duplicate_issue, canonical_issue)
create_issue_canonical_note(canonical_issue, duplicate_issue)
close_service.new(project, current_user, {}).execute(duplicate_issue)
close_service.new(project: project, current_user: current_user).execute(duplicate_issue)
duplicate_issue.update(duplicated_to: canonical_issue)
relate_two_issues(duplicate_issue, canonical_issue)
......
......@@ -61,7 +61,7 @@ module Issues
# Skip creation of system notes for existing attributes of the issue. The system notes of the old
# issue are copied over so we don't want to end up with duplicate notes.
CreateService.new(@target_project, @current_user, new_params).execute(skip_system_notes: true)
CreateService.new(project: @target_project, current_user: @current_user, params: new_params).execute(skip_system_notes: true)
end
def queue_copy_designs
......
......@@ -30,7 +30,7 @@ module Issues
def branches_with_merge_request_for(issue)
Issues::ReferencedMergeRequestsService
.new(project, current_user)
.new(project: project, current_user: current_user)
.referenced_merge_requests(issue)
.map(&:source_branch)
end
......
......@@ -21,7 +21,7 @@ module Issues
end
def update(issue, attrs)
::Issues::UpdateService.new(project, current_user, attrs).execute(issue)
::Issues::UpdateService.new(project: project, current_user: current_user, params: attrs).execute(issue)
rescue ActiveRecord::RecordNotFound
false
end
......
......@@ -122,7 +122,7 @@ module Issues
canonical_issue = IssuesFinder.new(current_user).find_by(id: canonical_issue_id)
if canonical_issue
Issues::DuplicateService.new(project, current_user).execute(issue, canonical_issue)
Issues::DuplicateService.new(project: project, current_user: current_user).execute(issue, canonical_issue)
end
end
# rubocop: enable CodeReuse/ActiveRecord
......@@ -135,7 +135,7 @@ module Issues
target_project != issue.project
update(issue)
Issues::MoveService.new(project, current_user).execute(issue, target_project)
Issues::MoveService.new(project: project, current_user: current_user).execute(issue, target_project)
end
private
......@@ -151,14 +151,14 @@ module Issues
# we've pre-empted this from running in #execute, so let's go ahead and update the Issue now.
update(issue)
Issues::CloneService.new(project, current_user).execute(issue, target_project, with_notes: with_notes)
Issues::CloneService.new(project: project, current_user: current_user).execute(issue, target_project, with_notes: with_notes)
end
def create_merge_request_from_quick_action
create_merge_request_params = params.delete(:create_merge_request)
return unless create_merge_request_params
MergeRequests::CreateFromIssueService.new(project, current_user, create_merge_request_params).execute
MergeRequests::CreateFromIssueService.new(project: project, current_user: current_user, mr_params: create_merge_request_params).execute
end
def handle_milestone_change(issue)
......
......@@ -2,10 +2,10 @@
module Issues
class ZoomLinkService < Issues::BaseService
def initialize(issue, user)
super(issue.project, user)
def initialize(project:, current_user:, params:)
super
@issue = issue
@issue = params.fetch(:issue)
@added_meeting = ZoomMeeting.canonical_meeting(@issue)
end
......
......@@ -35,7 +35,7 @@ module MergeRequests
end
def link_lfs_objects(merge_request)
LinkLfsObjectsService.new(merge_request.target_project).execute(merge_request)
LinkLfsObjectsService.new(project: merge_request.target_project).execute(merge_request)
end
end
end
......
# frozen_string_literal: true
module MergeRequests
class AssignIssuesService < BaseService
class AssignIssuesService < BaseProjectService
def assignable_issues
@assignable_issues ||= begin
if current_user == merge_request.author
......@@ -16,7 +16,7 @@ module MergeRequests
def execute
assignable_issues.each do |issue|
Issues::UpdateService.new(issue.project, current_user, assignee_ids: [current_user.id]).execute(issue)
Issues::UpdateService.new(project: issue.project, current_user: current_user, params: { assignee_ids: [current_user.id] }).execute(issue)
end
{
......
......@@ -147,7 +147,7 @@ module MergeRequests
if async
MergeRequests::CreatePipelineWorker.perform_async(project.id, user.id, merge_request.id)
else
MergeRequests::CreatePipelineService.new(project, user).execute(merge_request)
MergeRequests::CreatePipelineService.new(project: project, current_user: user).execute(merge_request)
end
end
......
......@@ -2,16 +2,28 @@
module MergeRequests
class CreateFromIssueService < MergeRequests::CreateService
def initialize(project, user, params)
# TODO: This constructor does not use the "params:" argument from the superclass,
# but instead has a custom "mr_params:" argument. This is because historically,
# prior to named arguments being introduced to the constructor, it never passed
# along the third positional argument when calling `super`.
# This should be changed, in order to be consistent (all subclasses should pass
# along all of the arguments to the superclass, otherwise it is probably not an
# "is a" relationship). However, we need to be sure that passing the params
# argument to `super` (especially target_project_id) will not cause any unexpected
# behavior in the superclass. Since the addition of the named arguments is
# intended to be a low-risk pure refactor, we will defer this fix
# to this follow-on issue:
# https://gitlab.com/gitlab-org/gitlab/-/issues/328726
def initialize(project:, current_user:, mr_params: {})
# branch - the name of new branch
# ref - the source of new branch.
@branch_name = params[:branch_name]
@issue_iid = params[:issue_iid]
@ref = params[:ref]
@target_project_id = params[:target_project_id]
@branch_name = mr_params[:branch_name]
@issue_iid = mr_params[:issue_iid]
@ref = mr_params[:ref]
@target_project_id = mr_params[:target_project_id]
super(project, user)
super(project: project, current_user: current_user)
end
def execute
......@@ -77,7 +89,7 @@ module MergeRequests
end
def merge_request
MergeRequests::BuildService.new(target_project, current_user, merge_request_params).execute
MergeRequests::BuildService.new(project: target_project, current_user: current_user, params: merge_request_params).execute
end
def merge_request_params
......
# frozen_string_literal: true
module MergeRequests
class GetUrlsService < BaseService
attr_reader :project
def initialize(project)
@project = project
end
class GetUrlsService < BaseProjectService
def execute(changes)
return [] unless project&.printing_merge_request_link_enabled
......
# frozen_string_literal: true
module MergeRequests
class LinkLfsObjectsService < ::BaseService
class LinkLfsObjectsService < ::BaseProjectService
def execute(merge_request, oldrev: merge_request.diff_base_sha, newrev: merge_request.diff_head_sha)
return if merge_request.source_project == project
return if no_changes?(oldrev, newrev)
......
......@@ -61,7 +61,7 @@ module MergeRequests
def squash_sha!
params[:merge_request] = merge_request
squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute
squash_result = ::MergeRequests::SquashService.new(project: project, current_user: current_user, params: params).execute
case squash_result[:status]
when :success
......
......@@ -17,7 +17,7 @@ module MergeRequests
def execute(merge_request, options = {})
if project.merge_requests_ff_only_enabled && !self.is_a?(FfMergeService)
FfMergeService.new(project, current_user, params).execute(merge_request)
FfMergeService.new(project: project, current_user: current_user, params: params).execute(merge_request)
return
end
......@@ -111,7 +111,7 @@ module MergeRequests
def after_merge
log_info("Post merge started on JID #{merge_jid} with state #{state}")
MergeRequests::PostMergeService.new(project, current_user).execute(merge_request)
MergeRequests::PostMergeService.new(project: project, current_user: current_user).execute(merge_request)
log_info("Post merge finished on JID #{merge_jid} with state #{state}")
if delete_source_branch?
......
......@@ -157,7 +157,7 @@ module MergeRequests
def merge_to_ref
params = { allow_conflicts: Feature.enabled?(:display_merge_conflicts_in_diff, project) }
result = MergeRequests::MergeToRefService.new(project, merge_request.author, params).execute(merge_request)
result = MergeRequests::MergeToRefService.new(project: project, current_user: merge_request.author, params: params).execute(merge_request)
result[:status] == :success
end
......
......@@ -44,7 +44,7 @@ module MergeRequests
closed_issues = merge_request.visible_closing_issues_for(current_user)
closed_issues.each do |issue|
Issues::CloseService.new(project, current_user).execute(issue, commit: merge_request)
Issues::CloseService.new(project: project, current_user: current_user).execute(issue, commit: merge_request)
end
end
......
# frozen_string_literal: true
module MergeRequests
class PushOptionsHandlerService
class PushOptionsHandlerService < ::BaseProjectService
LIMIT = 10
attr_reader :current_user, :errors, :changes,
:project, :push_options, :target_project
attr_reader :errors, :changes,
:push_options, :target_project
def initialize(project:, current_user:, params: {}, changes:, push_options:)
super(project: project, current_user: current_user, params: params)
def initialize(project, current_user, changes, push_options)
@project = project
@target_project = @project.default_merge_request_target
@current_user = current_user
@changes = Gitlab::ChangesList.new(changes)
@push_options = push_options
@errors = []
......@@ -95,16 +95,16 @@ module MergeRequests
# Use BuildService to assign the standard attributes of a merge request
merge_request = ::MergeRequests::BuildService.new(
project,
current_user,
create_params(branch)
project: project,
current_user: current_user,
params: create_params(branch)
).execute
unless merge_request.errors.present?
merge_request = ::MergeRequests::CreateService.new(
project,
current_user,
merge_request.attributes.merge(assignees: merge_request.assignees,
project: project,
current_user: current_user,
params: merge_request.attributes.merge(assignees: merge_request.assignees,
label_ids: merge_request.label_ids)
).execute
end
......@@ -114,9 +114,9 @@ module MergeRequests
def update!(merge_request)
merge_request = ::MergeRequests::UpdateService.new(
target_project,
current_user,
update_params(merge_request)
project: target_project,
current_user: current_user,
params: update_params(merge_request)
).execute(merge_request)
collect_errors_from_merge_request(merge_request) unless merge_request.valid?
......
......@@ -62,7 +62,7 @@ module MergeRequests
# the latest diff state as the last _valid_ one.
merge_requests_for_source_branch.reject(&:source_branch_exists?).each do |mr|
MergeRequests::CloseService
.new(mr.target_project, @current_user)
.new(project: mr.target_project, current_user: @current_user)
.execute(mr)
end
end
......@@ -96,7 +96,7 @@ module MergeRequests
merge_request.merge_commit_sha = analyzer.get_merge_commit(merge_request.diff_head_sha)
MergeRequests::PostMergeService
.new(merge_request.target_project, @current_user)
.new(project: merge_request.target_project, current_user: @current_user)
.execute(merge_request)
end
end
......@@ -109,7 +109,7 @@ module MergeRequests
merge_requests_for_forks.find_each do |mr|
LinkLfsObjectsService
.new(mr.target_project)
.new(project: mr.target_project)
.execute(mr, oldrev: @push.oldrev, newrev: @push.newrev)
end
end
......
......@@ -24,9 +24,11 @@ module MergeRequests
next unless can?(current_user, :update_merge_request, other_merge_request.source_project)
::MergeRequests::UpdateService
.new(other_merge_request.source_project, current_user,
target_branch: merge_request.target_branch,
target_branch_was_deleted: true)
.new(project: other_merge_request.source_project, current_user: current_user,
params: {
target_branch: merge_request.target_branch,
target_branch_was_deleted: true
})
.execute(other_merge_request)
end
end
......
......@@ -20,7 +20,7 @@ module MergeRequests
# Defer the more expensive operations (handle_assignee_changes) to the background
MergeRequests::HandleAssigneesChangeService
.new(project, current_user)
.new(project: project, current_user: current_user)
.async_execute(merge_request, old_assignees, execute_hooks: true)
merge_request
......
......@@ -4,7 +4,7 @@ module MergeRequests
class UpdateService < MergeRequests::BaseService
extend ::Gitlab::Utils::Override
def initialize(project, user = nil, params = {})
def initialize(project:, current_user: nil, params: {})
super
@target_branch_was_deleted = @params.delete(:target_branch_was_deleted)
......@@ -222,7 +222,7 @@ module MergeRequests
def handle_assignees_change(merge_request, old_assignees)
MergeRequests::HandleAssigneesChangeService
.new(project, current_user)
.new(project: project, current_user: current_user)
.async_execute(merge_request, old_assignees)
end
......@@ -304,11 +304,11 @@ module MergeRequests
def assignees_service
@assignees_service ||= ::MergeRequests::UpdateAssigneesService
.new(project, current_user, params)
.new(project: project, current_user: current_user, params: params)
end
def add_time_spent_service
@add_time_spent_service ||= ::MergeRequests::AddSpentTimeService.new(project, current_user, params)
@add_time_spent_service ||= ::MergeRequests::AddSpentTimeService.new(project: project, current_user: current_user, params: params)
end
end
end
......
......@@ -58,7 +58,7 @@ module Metrics
target_branch: project.default_branch,
title: params[:commit_message]
}
merge_request = ::MergeRequests::CreateService.new(project, current_user, merge_request_params).execute
merge_request = ::MergeRequests::CreateService.new(project: project, current_user: current_user, params: merge_request_params).execute
if merge_request.persisted?
success(result.merge(merge_request: Gitlab::UrlBuilder.build(merge_request)))
......
......@@ -7,11 +7,11 @@ module Milestones
update_params = { milestone: nil, skip_milestone_email: true }
milestone.issues.each do |issue|
Issues::UpdateService.new(parent, current_user, update_params).execute(issue)
Issues::UpdateService.new(project: parent, current_user: current_user, params: update_params).execute(issue)
end
milestone.merge_requests.each do |merge_request|
MergeRequests::UpdateService.new(parent, current_user, update_params).execute(merge_request)
MergeRequests::UpdateService.new(project: parent, current_user: current_user, params: update_params).execute(merge_request)
end
log_destroy_event_for(milestone)
......
......@@ -24,12 +24,12 @@ module Notes
UPDATE_SERVICES
end
def self.noteable_update_service(note)
def self.noteable_update_service_class(note)
update_services[note.noteable_type]
end
def self.supported?(note)
!!noteable_update_service(note)
!!noteable_update_service_class(note)
end
def supported?(note)
......@@ -55,7 +55,21 @@ module Notes
update_params[:spend_time][:note_id] = note.id
end
self.class.noteable_update_service(note).new(note.resource_parent, current_user, update_params).execute(note.noteable)
noteable_update_service_class = self.class.noteable_update_service_class(note)
# TODO: This conditional is necessary because we have not fully converted all possible
# noteable_update_service_class classes to use named arguments. See more details
# on the partial conversion at https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59182
# Follow-on issue to address this is here:
# https://gitlab.com/gitlab-org/gitlab/-/issues/328734
service =
if noteable_update_service_class.respond_to?(:constructor_container_arg)
noteable_update_service_class.new(**noteable_update_service_class.constructor_container_arg(note.resource_parent), current_user: current_user, params: update_params)
else
noteable_update_service_class.new(note.resource_parent, current_user, update_params)
end
service.execute(note.noteable)
end
end
end
......
......@@ -5,7 +5,7 @@ module Notes
def execute(note)
note.resolve!(current_user)
::MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
::MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(note.noteable)
end
end
end
......@@ -56,7 +56,7 @@ class PostReceiveService
end
service = ::MergeRequests::PushOptionsHandlerService.new(
project, user, changes, push_options
project: project, current_user: user, changes: changes, push_options: push_options
).execute
if service.errors.present?
......@@ -72,7 +72,7 @@ class PostReceiveService
def merge_request_urls
return [] unless repository&.repo_type&.project?
::MergeRequests::GetUrlsService.new(project).execute(params[:changes])
::MergeRequests::GetUrlsService.new(project: project).execute(params[:changes])
end
private
......
......@@ -17,7 +17,7 @@ module Projects
.from_and_to_forks(@project)
merge_requests.find_each do |mr|
::MergeRequests::CloseService.new(@project, @current_user).execute(mr)
::MergeRequests::CloseService.new(project: @project, current_user: @current_user).execute(mr)
log_info(message: "UnlinkForkService: Closed merge request", merge_request_id: mr.id)
end
......
......@@ -17,7 +17,7 @@ module Ci
return unless job && project
::MergeRequests::AddTodoWhenBuildFailsService.new(job.project, nil).execute(job)
::MergeRequests::AddTodoWhenBuildFailsService.new(project: job.project).execute(job)
end
end
end
......
......@@ -33,7 +33,7 @@ class IssuePlacementWorker
leftover = to_place.pop if to_place.count > QUERY_LIMIT
Issue.move_nulls_to_end(to_place)
Issues::BaseService.new(nil).rebalance_if_needed(to_place.max_by(&:relative_position))
Issues::BaseService.new(project: nil).rebalance_if_needed(to_place.max_by(&:relative_position))
IssuePlacementWorker.perform_async(nil, leftover.project_id) if leftover.present?
rescue RelativePositioning::NoSpaceLeft => e
Gitlab::ErrorTracking.log_exception(e, issue_id: issue_id, project_id: project_id)
......
......@@ -21,7 +21,7 @@ class MergeRequests::AssigneesChangeWorker
return if users.blank?
::MergeRequests::HandleAssigneesChangeService
.new(merge_request.target_project, current_user)
.new(project: merge_request.target_project, current_user: current_user)
.execute(merge_request, users, execute_hooks: true)
rescue ActiveRecord::RecordNotFound
end
......
......@@ -23,7 +23,7 @@ module MergeRequests
merge_request = MergeRequest.find_by_id(merge_request_id)
return unless merge_request
MergeRequests::CreatePipelineService.new(project, user).execute(merge_request)
MergeRequests::CreatePipelineService.new(project: project, current_user: user).execute(merge_request)
merge_request.update_head_pipeline
end
end
......
......@@ -19,7 +19,7 @@ class MergeRequests::DeleteSourceBranchWorker
::Branches::DeleteService.new(merge_request.source_project, user)
.execute(merge_request.source_branch)
::MergeRequests::RetargetChainService.new(merge_request.source_project, user)
::MergeRequests::RetargetChainService.new(project: merge_request.source_project, current_user: user)
.execute(merge_request)
rescue ActiveRecord::RecordNotFound
end
......
......@@ -17,7 +17,7 @@ class MergeRequests::HandleAssigneesChangeWorker
old_assignees = User.id_in(old_assignee_ids)
::MergeRequests::HandleAssigneesChangeService
.new(merge_request.target_project, user)
.new(project: merge_request.target_project, current_user: user)
.execute(merge_request, old_assignees, options)
rescue ActiveRecord::RecordNotFound
end
......
......@@ -23,7 +23,7 @@ class MergeWorker # rubocop:disable Scalability/IdempotentWorker
return
end
MergeRequests::MergeService.new(merge_request.target_project, current_user, params)
MergeRequests::MergeService.new(project: merge_request.target_project, current_user: current_user, params: params)
.execute(merge_request)
end
end
......@@ -20,7 +20,7 @@ class NewIssueWorker # rubocop:disable Scalability/IdempotentWorker
issuable.create_cross_references!(user)
Issues::AfterCreateService
.new(issuable.project, user)
.new(project: issuable.project, current_user: user)
.execute(issuable)
end
......
......@@ -15,7 +15,7 @@ class NewMergeRequestWorker # rubocop:disable Scalability/IdempotentWorker
return unless objects_found?(merge_request_id, user_id)
MergeRequests::AfterCreateService
.new(issuable.target_project, user)
.new(project: issuable.target_project, current_user: user)
.execute(issuable)
end
......
......@@ -53,7 +53,7 @@ class ProcessCommitWorker
# therefore we use IssueCollection here and skip the authorization check in
# Issues::CloseService#execute.
IssueCollection.new(issues).updatable_by_user(user).each do |issue|
Issues::CloseService.new(project, author)
Issues::CloseService.new(project: project, current_user: author)
.close_issue(issue, closed_via: commit)
end
end
......
......@@ -16,7 +16,7 @@ class RebaseWorker # rubocop:disable Scalability/IdempotentWorker
merge_request = MergeRequest.find(merge_request_id)
MergeRequests::RebaseService
.new(merge_request.source_project, current_user)
.new(project: merge_request.source_project, current_user: current_user)
.execute(merge_request, skip_ci: skip_ci)
end
end
......@@ -19,7 +19,7 @@ class UpdateMergeRequestsWorker # rubocop:disable Scalability/IdempotentWorker
user = User.find_by(id: user_id)
return unless user
MergeRequests::RefreshService.new(project, user).execute(oldrev, newrev, ref)
MergeRequests::RefreshService.new(project: project, current_user: user).execute(oldrev, newrev, ref)
end
# rubocop: enable CodeReuse/ActiveRecord
end
......@@ -36,7 +36,7 @@ Gitlab::Seeder.quiet do
break unless developer
Sidekiq::Worker.skipping_transaction_check do
MergeRequests::CreateService.new(project, developer, params).execute
MergeRequests::CreateService.new(project: project, current_user: developer, params: params).execute
rescue Repository::AmbiguousRefError
# Ignore pipelines creation errors for now, we can doing that after
# https://gitlab.com/gitlab-org/gitlab-foss/issues/55966. will be resolved.
......@@ -55,7 +55,7 @@ Gitlab::Seeder.quiet do
title: 'Can be automatically merged'
}
Sidekiq::Worker.skipping_transaction_check do
MergeRequests::CreateService.new(project, User.admins.first, params).execute
MergeRequests::CreateService.new(project: project, current_user: User.admins.first, params: params).execute
end
print '.'
......@@ -65,7 +65,7 @@ Gitlab::Seeder.quiet do
title: 'Cannot be automatically merged'
}
Sidekiq::Worker.skipping_transaction_check do
MergeRequests::CreateService.new(project, User.admins.first, params).execute
MergeRequests::CreateService.new(project: project, current_user: User.admins.first, params: params).execute
end
print '.'
end
......@@ -40,7 +40,7 @@ class Groups::EpicsController < Groups::ApplicationController
end
def create
@epic = ::Epics::CreateService.new(@group, current_user, epic_params).execute
@epic = ::Epics::CreateService.new(group: @group, current_user: current_user, params: epic_params).execute
if @epic.persisted?
render json: {
......@@ -102,7 +102,7 @@ class Groups::EpicsController < Groups::ApplicationController
end
def update_service
::Epics::UpdateService.new(@group, current_user, epic_params.to_h)
::Epics::UpdateService.new(group: @group, current_user: current_user, params: epic_params.to_h)
end
def finder_type
......
......@@ -21,7 +21,7 @@ module Mutations
validate_arguments!(args)
group = authorized_find!(group_path: group_path)
epic = ::Epics::CreateService.new(group, current_user, args).execute
epic = ::Epics::CreateService.new(group: group, current_user: current_user, params: args).execute
response_object = epic if epic.valid?
......
......@@ -21,7 +21,7 @@ module Mutations
validate_arguments!(args)
epic = authorized_find!(group_path: group_path, iid: epic_iid)
epic = ::Epics::UpdateService.new(epic.group, current_user, args).execute(epic)
epic = ::Epics::UpdateService.new(group: epic.group, current_user: current_user, params: args).execute(epic)
{
epic: epic.reset,
......
......@@ -23,7 +23,7 @@ module Mutations
group = get_group_by_path!(group_path)
begin
epic = ::Epics::IssuePromoteService.new(project, current_user).execute(issue, group)
epic = ::Epics::IssuePromoteService.new(project: project, current_user: current_user).execute(issue, group)
rescue StandardError => error
errors << error.message
end
......
......@@ -18,7 +18,7 @@ module Mutations
authorize_admin_rights!(epic)
::Issues::UpdateService.new(project, current_user, epic: epic)
::Issues::UpdateService.new(project: project, current_user: current_user, params: { epic: epic })
.execute(issue)
{
......
......@@ -10,14 +10,14 @@ module Mutations
required: false,
loads: Types::IterationType,
description: <<~DESC
The iteration to assign to the issue.
The iteration to assign to the issue.
DESC
def resolve(project_path:, iid:, iteration: nil)
issue = authorized_find!(project_path: project_path, iid: iid)
project = issue.project
::Issues::UpdateService.new(project, current_user, iteration: iteration)
::Issues::UpdateService.new(project: project, current_user: current_user, params: { iteration: iteration })
.execute(issue)
{
......
......@@ -14,7 +14,7 @@ module Mutations
issue = authorized_find!(project_path: project_path, iid: iid)
project = issue.project
::Issues::UpdateService.new(project, current_user, weight: weight)
::Issues::UpdateService.new(project: project, current_user: current_user, params: { weight: weight })
.execute(issue)
{
......
......@@ -14,9 +14,9 @@ module Mutations
project = authorized_find!(project_path)
requirement = ::RequirementsManagement::CreateRequirementService.new(
project,
context[:current_user],
args
project: project,
current_user: context[:current_user],
params: args
).execute
{
......
......@@ -8,7 +8,7 @@ module Boards
private
def update(epic, epic_modification_params)
::Epics::UpdateService.new(epic.group, current_user, epic_modification_params).execute(epic)
::Epics::UpdateService.new(group: epic.group, current_user: current_user, params: epic_modification_params).execute(epic)
end
def board
......
......@@ -17,7 +17,7 @@ module EE
return unless epic = original_entity.epic
return unless can?(current_user, :update_epic, epic.group)
updated = ::Issues::UpdateService.new(target_project, current_user, epic: epic).execute(new_entity)
updated = ::Issues::UpdateService.new(project: target_project, current_user: current_user, params: { epic: epic }).execute(new_entity)
::Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_changed_epic_action(author: current_user) if updated
end
......
......@@ -64,7 +64,7 @@ module EE
def handle_promotion(issue)
return unless params.delete(:promote_to_epic)
Epics::IssuePromoteService.new(issue.project, current_user).execute(issue)
Epics::IssuePromoteService.new(project: issue.project, current_user: current_user).execute(issue)
end
end
end
......
......@@ -4,9 +4,50 @@ module Epics
class BaseService < IssuableBaseService
extend ::Gitlab::Utils::Override
def self.constructor_container_arg(value)
# TODO: Dynamically determining the type of a constructor arg based on the class is an antipattern,
# but the root cause is that Epics::BaseService has some issues that inheritance may not be the
# appropriate pattern. See more details in comments at the top of Epics::BaseService#initialize.
# Follow on issue to address this:
# https://gitlab.com/gitlab-org/gitlab/-/issues/328438
{ group: value }
end
attr_reader :group, :parent_epic, :child_epic
def initialize(group, current_user, params = {})
# TODO: This constructor does NOT call `super`, because it has
# no `project` associated. Thus, the first argument is named
# `group`, even though it only a `group` in this sub-hierarchy of `IssuableBaseClass`,
# but is a `project` everywhere else. This is because named arguments
# were added after the class was already in use. We use `.constructor_container_arg`
# to determine the correct keyword to use.
#
# This is revealing an inconsistency which already existed,
# where sometimes a `project` is passed as the first argument but ignored. For example,
# in `IssuableBaseService#change_state` method, as well as many others.
#
# This is a form of violation of the Liskov Substitution Principle
# (https://en.wikipedia.org/wiki/Liskov_substitution_principle),
# in that we cannot determine which form of the constructor to call without
# knowing what the type of subclass is.
#
# This implies that inheritance may not be the proper relationship to "issuable",
# because it may not be an "is a" relationship.
#
# All other `IssuableBaseService` subclasses are in the context of a
# project, and take the project as the first argument to the constructor.
#
# Instead, is seems like there is are some concerns such as state management, and
# having notes, which are applicable to "epic" services, but not necessarily all aspects
# of "issuable" services.
#
# See the following links for more context:
# - Original discussion thread: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59182#note_555401711
# - Issue to address inheritance problems: https://gitlab.com/gitlab-org/gitlab/-/issues/328438
def initialize(group:, current_user:, params: {})
# NOTE: this does NOT call `super`! See details in comment above.
@group = group
@current_user = current_user
@params = params
......
......@@ -39,7 +39,7 @@ module Epics
end
def create_new_entity
@new_entity = Epics::CreateService.new(parent_group, current_user, params).execute
@new_entity = Epics::CreateService.new(group: parent_group, current_user: current_user, params: params).execute
end
def update_old_entity
......
......@@ -46,7 +46,7 @@ module Epics
epic_params = epic.attributes
.slice('title', 'description', 'start_date', 'end_date', 'confidential')
CreateService.new(project.group, current_user, epic_params).execute
CreateService.new(group: project.group, current_user: current_user, params: epic_params).execute
end
# rubocop: disable CodeReuse/ActiveRecord
......
......@@ -17,7 +17,7 @@ module Issues
confidential: true
}
issue = Issues::CreateService.new(@project, @current_user, issue_params).execute
issue = Issues::CreateService.new(project: @project, current_user: @current_user, params: issue_params).execute
if issue.valid?
success(issue)
......
......@@ -31,7 +31,7 @@ module MergeRequests
force_remove_source_branch: "1"
}
merge_request = MergeRequests::CreateService.new(@project, @current_user, merge_request_params).execute
merge_request = MergeRequests::CreateService.new(project: @project, current_user: @current_user, params: merge_request_params).execute
if merge_request.valid?
success(merge_request)
......
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
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