<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/contributor-success/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Close this issue](https://contributors.gitlab.com/manage-issue?action=close&projectId=278964&issueIid=17291) </details> <!--IssueSummary end--> # What it is `MergeRequests::BuildService` is used to determine if we're going to show the preview of a new merge request, pre-filling some data in the form and ask the user to enter the other information in order to create a merge request. # What to do Following the conversation from https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9263#note_23507289, I propose that we: * Remove `MergeRequest#can_be_created` * Move all the checks in `MergeRequests::BuildService` to be validations on `MergeRequest` * Replace the check to `MergeRequest#can_be_created` to be `MergeRequest#valid?` * Make `MergeRequests::BuildService` only pre-fill data and handle error messages if extra action needed. The validation could look something like: ``` ruby # Note that the below check should happen after `validate_branches` and so on validate :validate_commit_existence, on: :create def validate_commit_existence unless source_project.commit(source_branch) errors.add(:source_commit, '...') end unless target_project.commit(target_branch) errors.add(:target_commit, '...') end end ``` And update the error messages accordingly. # Rationale behind this `MergeRequest#can_be_created` is really a bad way to pass data. The way it is right now is like saving a local variable in another object, while the object saving the data has no idea what it is. Ideally, validations should not be a state in the record, but the validation itself. Something like: ``` ruby valid = Validate::MergeRequest.new(merge_request).validate ``` Would work much better because if we're ever going to validate twice (or use a different validation because, see `MergeRequest#allow_broken`), we just need to create a new validation object instead of clearing the state on the record. This works better: ``` ruby valid = Validate::MergeRequest.new(merge_request).validate merge_request.update(params) valid_again = Validate::MergeRequest.new(merge_request).validate ``` Than this: ``` ruby valid = merge_request.valid? merge_request.update(params) merge_request.reset_validation # BOO valid_again = merge_request.valid? ``` However we're just stuck with Rails, and in this particular case, the object, that is the merge request should totally know if the branches are valid or not, which is the only check used for `MergeRequest#can_be_created`. Checking the other validations used in `MergeRequest`: * `validates :source_project, presence: true` Of course we also want this when previewing * `validates :source_branch, presence: true` Same * `validates :target_project, presence: true` Same * `validates :target_branch, presence: true` Same * `validate :validate_branches` Why are we doing similar thing in `MergeRequests::BuildService` too? This is where we should merge the checks. * `validate :validate_fork` This won't hurt That means we certainly also want the above to pass even if we're previewing the merge request. All the other `attr_accessor` should probably also be removed from `MergeRequest`, but each one would have their own story and would be another topic. /cc @felipe_artur @smcgivern p.s. `MergeRequests::BuildService` is really in a very confusing state, probably only better than `CreatesCommit`