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

What does this MR do?

What

• Continuation of refactoring discussed and started in !30979 (comment 337747596).
• Refactors Issuable-related service classes to subclass BaseContainerService instead of BaseService, via new Projects::BaseProjectService.
• Specifically, the following class hierarchies were changed to subclass the new Projects::BaseProjectService:
  • IssuableBaseService and all subclasses
  • MergeRequests::AssignIssuesService and MergeRequests::GetUrlsService (for consistency with all other classes in app/services/merge_requests dir)
• Projects::BaseProjectService#initialize and subclasses take project: as the first named argument, however...
  • The Epics::BaseService is an exception to this - it takes group: as the first arg. This should be refactored, because it does not call super, indicating inheritance may not be appropriate here.
  • This required the introduction of .noteable_update_service_class on the superclasses, in order to determine the correct argument to pass.
  • See the following links for more context:
    • Original discussion thread: !59182 (comment 555401711)
    • Issue to address inheritance problems: #328438

Why

This provides a greater degree of type safety in order to more easily and safely introduce a new named argument spam_params to support !58603 (merged).

The additional type safety will also make future refactors easier and less risky, such as https://gitlab.com/gitlab-org/architecture/tasks/-/issues/7 and !54792 (merged).

Is this change risky?

Not really.

First of all, this is a pure refactoring to the constructors of a single superclass hierarchy. There are no business logic or behavioral changes.

Also, since this change is adding type safety to constructors, it's relatively low-risk.

This because the changed classes can't even be instantiated if they were not correctly converted to the required named arguments.

Therefore, the only way the pipeline could be green while error still exist is if there is no test coverage, anywhere in the entire test suite, that instantiates and asserts any successful use the class

In fact, if there is such a big hole in our test coverage, and a bug is uncovered after this MR is merged, we want to know about it so we can backfill appropriate test coverage 😉

Do we have any idea if there IS any missing test coverage?

Yes, if you go to the "Changes" tab of this MR, you can open the "Elements" tab of the Chrome Javascript console, and search for "No test coverage".

Here, you can see that there are no modified lines which are lacking test coverage. The only line lacking test coverage is app/graphql/mutations/issues/create.rb:79, which was not modified in this MR.

So what are the riskiest parts?

These are the only pieces of actual logic (conditionals or new methods) which have been added or changed, other than the constructors, calls to constructors, and the addition of a couple of additional constructors in the hierarchy:

1. The addition of the .noteable_update_service_class discussed above. This is to determine the correct argument name to pass in the case of Epics::BaseService subclasses. But again, if this is wrong, the class cannot be instantiated, and as long as any test attempts to use and verify the classes behavior, it will fail.
2. A single conditional added to the bottom of app/services/notes/quick_actions_service.rb, to account for the fact that it needs to instantiate some services outside the IssueableBaseService hierarchy which have not yet had named arguments added.

This MR is very big! Can't it be broken up?

Yes, but it all is part of a cohesive refactor of the IssuableBaseClass constructor and associated subclass changes. There's just a lot of subclasses.

I don't see a clear place it should be broken up - it was a matter of just finding all the failing tests and fixing them.

In fact, trying to convert only part of the hierarchy could end up being more risky, because it would then likely end up not being a pure refactor, because we would have to introduce new code which was otherwise unnecessary. For example, new constructors at arbitrary branches of the class hierarchy, and possibly additional conditional logic to handle the different types of constructors when they are used dynamically (such as we had to do in app/services/notes/quick_actions_service.rb.

And even if we did that, it would involve a lot more busy-work of finding and changing hundreds of signatures (and this MR has already taken several days of work to get green).

I'm open to suggestions on how to easily break it up, though.

Screenshots (strongly suggested)

Refactored class hierarchy

Expand to see class hierarchy changed, or view screenshot below:

Summary of class hierarchy changed

Projects::BaseProjectService (base_project_service.rb)
    Issuable::CommonSystemNotesService (common_system_notes_service.rb)
    RequirementsManagement::CreateRequirementService (create_requirement_service.rb)
    MergeRequests::AssignIssuesService (assign_issues_service.rb)
    MergeRequests::GetUrlsService (get_urls_service.rb)
    IssuableBaseService (issuable_base_service.rb)
        Issuable::Clone::BaseService (base_service.rb)
            Issues::CloneService (clone_service.rb)
            Epics::IssuePromoteService (issue_promote_service.rb)
            Issues::MoveService (move_service.rb)
            Issuable::Clone::AttributesRewriter (attributes_rewriter.rb)
        MergeRequests::BaseService (base_service.rb)
            MergeRequests::AddTodoWhenBuildFailsService (add_todo_when_build_fails_service.rb)
            MergeRequests::BuildService (build_service.rb)
            MergeRequests::RefreshService (refresh_service.rb)
            MergeRequests::RemoveApprovalService (remove_approval_service.rb)
            MergeRequests::ReopenService (reopen_service.rb)
            MergeRequests::UpdateService (update_service.rb)
                MergeRequests::UpdateAssigneesService (update_assignees_service.rb)
            MergeRequests::AddContextService (add_context_service.rb)
            MergeRequests::MergeBaseService (merge_base_service.rb)
                MergeRequests::MergeToRefService (merge_to_ref_service.rb)
                MergeRequests::MergeService (merge_service.rb)
                    MergeRequests::FfMergeService (ff_merge_service.rb)
                MergeRequests::FfMergeService (ff_merge_service.rb)
            MergeRequests::PostMergeService (post_merge_service.rb)
            MergeRequests::RetargetChainService (retarget_chain_service.rb)
            MergeRequests::CloseService (close_service.rb)
            MergeRequests::SquashService (squash_service.rb)
            MergeRequests::RequestReviewService (request_review_service.rb)
            MergeRequests::AfterCreateService (after_create_service.rb)
            MergeRequests::MarkReviewerReviewedService (mark_reviewer_reviewed_service.rb)
            EE::MergeRequests::ResetApprovalsService (reset_approvals_service.rb)
            MergeRequests::CreateService (create_service.rb)
                MergeRequests::CreateFromIssueService (create_from_issue_service.rb)
            MergeRequests::CreatePipelineService (create_pipeline_service.rb)
            MergeRequests::HandleAssigneesChangeService (handle_assignees_change_service.rb)
            MergeRequests::RebaseService (rebase_service.rb)
            MergeRequests::ApprovalService (approval_service.rb)
            MergeRequests::ResolvedDiscussionNotificationService (resolved_discussion_notification_service.rb)
            MergeRequests::PushedBranchesService (pushed_branches_service.rb)
            MergeRequests::UpdateAssigneesService (update_assignees_service.rb)
            MergeRequests::MergeToRefService (merge_to_ref_service.rb)
            MergeRequests::MergeService (merge_service.rb)
                MergeRequests::FfMergeService (ff_merge_service.rb)
            MergeRequests::CreateFromIssueService (create_from_issue_service.rb)
            MergeRequests::FfMergeService (ff_merge_service.rb)
        Epics::BaseService (base_service.rb)
            Epics::CreateService (create_service.rb)
            Epics::CloseService (close_service.rb)
            Epics::TreeReorderService (tree_reorder_service.rb)
            Epics::ReopenService (reopen_service.rb)
            Epics::UpdateService (update_service.rb)
        Issues::BaseService (base_service.rb)
            Issues::UpdateService (update_service.rb)
            Issues::CloseService (close_service.rb)
            Issues::ReopenService (reopen_service.rb)
            Issues::AfterCreateService (after_create_service.rb)
            Issues::ReorderService (reorder_service.rb)
            Issues::BuildService (build_service.rb)
                Issues::BuildFromVulnerabilityService (build_from_vulnerability_service.rb)
            Issues::RelatedBranchesService (related_branches_service.rb)
            Issues::ReferencedMergeRequestsService (referenced_merge_requests_service.rb)
            Issues::ZoomLinkService (zoom_link_service.rb)
            Issues::DuplicateService (duplicate_service.rb)
            Issues::CreateService (create_service.rb)
            Issues::BuildFromVulnerabilityService (build_from_vulnerability_service.rb)
        Issuable::DestroyService (destroy_service.rb)
        Issues::CloneService (clone_service.rb)
        Epics::IssuePromoteService (issue_promote_service.rb)
        Issues::MoveService (move_service.rb)
        Issuable::Clone::AttributesRewriter (attributes_rewriter.rb)
        MergeRequests::AddTodoWhenBuildFailsService (add_todo_when_build_fails_service.rb)
        MergeRequests::BuildService (build_service.rb)
        MergeRequests::RefreshService (refresh_service.rb)
        MergeRequests::RemoveApprovalService (remove_approval_service.rb)
        MergeRequests::ReopenService (reopen_service.rb)
        MergeRequests::UpdateService (update_service.rb)
            MergeRequests::UpdateAssigneesService (update_assignees_service.rb)
        MergeRequests::AddContextService (add_context_service.rb)
        MergeRequests::MergeBaseService (merge_base_service.rb)
            MergeRequests::MergeToRefService (merge_to_ref_service.rb)
            MergeRequests::MergeService (merge_service.rb)
                MergeRequests::FfMergeService (ff_merge_service.rb)
            MergeRequests::FfMergeService (ff_merge_service.rb)
        MergeRequests::PostMergeService (post_merge_service.rb)
        MergeRequests::RetargetChainService (retarget_chain_service.rb)
        MergeRequests::CloseService (close_service.rb)
        MergeRequests::SquashService (squash_service.rb)
        MergeRequests::RequestReviewService (request_review_service.rb)
        MergeRequests::AfterCreateService (after_create_service.rb)
        MergeRequests::MarkReviewerReviewedService (mark_reviewer_reviewed_service.rb)
        EE::MergeRequests::ResetApprovalsService (reset_approvals_service.rb)
        MergeRequests::CreateService (create_service.rb)
            MergeRequests::CreateFromIssueService (create_from_issue_service.rb)
        MergeRequests::CreatePipelineService (create_pipeline_service.rb)
        MergeRequests::HandleAssigneesChangeService (handle_assignees_change_service.rb)
        MergeRequests::RebaseService (rebase_service.rb)
        MergeRequests::ApprovalService (approval_service.rb)
        MergeRequests::ResolvedDiscussionNotificationService (resolved_discussion_notification_service.rb)
        MergeRequests::PushedBranchesService (pushed_branches_service.rb)
        Epics::CreateService (create_service.rb)
        Epics::CloseService (close_service.rb)
        Epics::TreeReorderService (tree_reorder_service.rb)
        Epics::ReopenService (reopen_service.rb)
        Epics::UpdateService (update_service.rb)
        Issues::UpdateService (update_service.rb)
        Issues::CloseService (close_service.rb)
        Issues::ReopenService (reopen_service.rb)
        Issues::AfterCreateService (after_create_service.rb)
        Issues::ReorderService (reorder_service.rb)
        Issues::BuildService (build_service.rb)
            Issues::BuildFromVulnerabilityService (build_from_vulnerability_service.rb)
        Issues::RelatedBranchesService (related_branches_service.rb)
        Issues::ReferencedMergeRequestsService (referenced_merge_requests_service.rb)
        Issues::ZoomLinkService (zoom_link_service.rb)
        Issues::DuplicateService (duplicate_service.rb)
        Issues::CreateService (create_service.rb)
        MergeRequests::UpdateAssigneesService (update_assignees_service.rb)
        MergeRequests::MergeToRefService (merge_to_ref_service.rb)
        MergeRequests::MergeService (merge_service.rb)
            MergeRequests::FfMergeService (ff_merge_service.rb)
        MergeRequests::CreateFromIssueService (create_from_issue_service.rb)
        Issues::BuildFromVulnerabilityService (build_from_vulnerability_service.rb)
        MergeRequests::FfMergeService (ff_merge_service.rb)
    MergeRequests::LinkLfsObjectsService (link_lfs_objects_service.rb)
    MergeRequests::PushOptionsHandlerService (push_options_handler_service.rb)

Rollout Plan

• 2021-05-10 - Announce in engineering week in review
• 2021-05-10 - Announce in Slack #s_plan and #backend
• 2021-05-11 - Do a final rebase and ensure pipeline is green
• 2021-05-12 - Merge!
• 2021-05-12 - Review master merge pipeline for any downstream errors: https://gitlab.com/gitlab-org/gitlab/-/pipelines/301302236
• 2021-05-12 thru 2021-05-19 - Monitor#is-this-known and #master-broken, #production, and #dev-escalation for any reported problems

Does this MR meet the acceptance criteria?

Conformity

• 📋 Does this MR need a changelog?
  • [-] I have included a changelog entry.
  • I have not included a changelog entry because it is a pure refactor, there are no user facing changes.
• [-] Documentation (if required)
• Code review guidelines
• Merge request performance guidelines
• [-] Style guides
• [-] Database guides
• Separation of EE specific content

Availability and Testing

See sections above for a detailed discussion of the risk.

• Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
• [-] Tested in all supported browsers
• [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

• [-] Label as security and @ mention @gitlab-com/gl-security/appsec
• [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
• [-] Security reports checked/validated by a reviewer from the AppSec team