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 bothMergeRequestsController
andDiffsController
. 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 fromMergeRequests::ApplicationController
have access to#commit
even if they don't need it, I think it's better.