[go: up one dir, main page]

Skip to content

Introduce a controller concern for finding commit in Merge Request controllers

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

The following discussion from !173382 (merged) should be addressed:

  • @patrickbajao started a discussion: (+2 comments)

    I was thinking about this more today and I feel that it's better to have a controller concern something like WithMergeRequestCommit and we move the #commit method there. We can then include this concern in both MergeRequestsController and DiffsController. It would look something like:

    module WithMergeRequestCommit
      def commit
        commit_id = params[:commit_id].presence
        return unless commit_id
    
        return unless @merge_request.all_commits.exists?(sha: commit_id) ||
          @merge_request.recent_context_commits.map(&:id).include?(commit_id)
    
        @commit ||= @project.commit(commit_id)
      end
    end
    
    class DiffsController
      include WithMergeRequestCommit
    end
    
    class MergeRequestsController
      include WithMergeRequestCommit
    end

    Doing it this way means if a controller needs the #commit method they can include the concern. Compared to what we have now wherein all controllers inheriting from MergeRequests::ApplicationController have access to #commit even if they don't need it, I think it's better.

Edited by 🤖 GitLab Bot 🤖