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
WithMergeRequestCommitand we move the#commitmethod there. We can then include this concern in bothMergeRequestsControllerandDiffsController. 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 endDoing it this way means if a controller needs the
#commitmethod they can include the concern. Compared to what we have now wherein all controllers inheriting fromMergeRequests::ApplicationControllerhave access to#commiteven if they don't need it, I think it's better.