Address follow-up questions/suggestions from "Update GitHub pull request importers to handle user mapping"

The following discussions from !171340 (merged) should be addressed:

Questions

@.luke started a discussion:

Question

Why do we not add this note when using UCM? We currently always add it, but with now with UCM we would never add it.

Suggestions

@SamWord started a discussion: (+1 comment)

This was causing flaky specs if I didn't reset user_contribution_mapping_enabled in an after block. I'm not entirely sure why this doesn't seem to be an issue in the other specs that call project.build_or_assign_import_data to enable UCM in other specs, but it makes me a little suspicious about using it

Suggestion

@.luke made a suggestion on how to fix the flaky specs without the after block !171340 (comment 2212255643)

Suggestion

Instead of inserting and then finding the record, it might be best to .create now, what do you think? It looks like the specs expect to return if the approval is not unique, so in that case we use .create rather than .create!.

        attributes = {
          merge_request_id: merge_request_id,
          user_id: user_id,
          created_at: submitted_at,
          updated_at: submitted_at,
          importing: true
        }

        approval = Approval.create(attributes)

        return unless approval.persisted?

        note = add_approval_system_note!(project_id, merge_request_id, user_id, submitted_at)

Non-blocking suggestions

@.luke started a discussion:
Suggestion (not blocking)

Minor, but we access the properties using . elsewhere, so for consistency we could do the same here:
```
              push_with_record(merge_request.metrics, :merged_by_id, merged_by.id, mapper.user_mapper)
```
@.luke started a discussion:
Suggestion (not blocking)

Should we add a comment to remove this when the UCM flag is removed? It could be missed otherwise and the if/else could be around in the codebase for a while before someone notices.
```
              # TODO this method, and the if/else can be removed when `github_user_mapping` flag is removed.
              add_complementary_review_note!(project.creator_id)
```
@.luke started a discussion:
Suggestion (not blocking)

Minor but there's a neat rspec DSL for this .to all():
```
    expect(reviewers).to all(be_placeholder)
```
@.luke started a discussion:
Suggestion (not blocking)

Minor, but we don't need to access through ::Import in this spec file, and other specs too, we should always be able to just:
```
    user_references = placeholder_user_references(Import::SOURCE_GITHUB, project.import_state.id)
```
@.luke started a discussion:
Suggestion (not blocking)

We could turn user_references into a let (or method if we want to call it without memoization):
```
let(:user_references) { placeholder_user_references(::Import::SOURCE_GITHUB, project.import_state.id) }
```
and then we could remove the 7 lines like this in this file:
@.luke started a discussion:

Suggestion (not blocking)

This context is acting like a bucket of specs that test behaviours that don't relate to whether user mapping is enabled or disabled 🤔. It feels like now that we're not stubbing MR creation in the #execute test that we could probably move these tests out of the #create_merge_request test and into the #execute test and make #create_merge_request and other methods like it private in the importer, and stop testing them directly and instead test their behaviours through the public #execute method 🤔.

Edited Nov 15, 2024 by Luke Duncalfe