feat(pull_requests): add web UI for pull request management

index 7f8c0ab..e180823 100644 --- a/app/assets/stylesheets/application.tailwind.css +++ b/app/assets/stylesheets/application.tailwind.css @@ -71,8 +71,11 @@ .badge { @apply inline-flex items-center px-2 py-0.5 rounded text-xs font-medium; } - .badge-gray { @apply badge bg-surface-600 text-gray-300; } - .badge-blue { @apply badge bg-brand-500/10 text-brand-400; } + .badge-gray { @apply badge bg-surface-600 text-gray-300; } + .badge-blue { @apply badge bg-brand-500/10 text-brand-400; } + .badge-green { @apply badge bg-green-900/30 text-green-400; } + .badge-red { @apply badge bg-red-900/30 text-red-400; } + .badge-purple { @apply badge bg-purple-900/30 text-purple-400; } .tab-item { @apply px-4 py-2.5 text-sm font-medium text-gray-500 border-b-2 border-transparent

new file mode 100644 index 0000000..12c5130 --- /dev/null +++ b/app/controllers/pull_requests_controller.rb @@ -0,0 +1,120 @@ +class PullRequestsController < ApplicationController + helper_method :render_markdown + + before_action :load_owner + before_action :load_repository + before_action :ensure_can_read! + before_action :require_sign_in!, only: %i[new create close reopen merge add_comment] + before_action :load_pull_request, only: %i[show close reopen merge add_comment] + + def index + @state = %w[open closed merged all].include?(params[:state]) ? params[:state] : "open" + scope = @repository.pull_requests + scope = scope.where(state: @state) unless @state == "all" + @pull_requests = scope.order(number: :desc).includes(:user) + + @open_count = @repository.pull_requests.open.count + @closed_count = @repository.pull_requests.closed.count + @merged_count = @repository.pull_requests.merged.count + end + + def new + @branches = GitRepositoryService.branches(@repository.disk_path) + @pull_request = @repository.pull_requests.new( + head_ref: params[:head], base_ref: params[:base].presence || @repository.default_branch + ) + end + + def create + pr_params = params.require(:pull_request).permit(:title, :body, :head_ref, :base_ref) + @pull_request = PullRequestService.create( + repository: @repository, author: current_user, + title: pr_params[:title], head: pr_params[:head_ref], base: pr_params[:base_ref], body: pr_params[:body] + ) + redirect_to repository_pull_request_path(@owner.username, @repository.name, @pull_request.number), + notice: "Pull request ##{@pull_request.number} opened." + rescue PullRequestService::NotAuthorized => e + redirect_to repository_path(@owner.username, @repository.name), alert: e.message + rescue PullRequestService::Error, ActiveRecord::RecordInvalid => e + @branches = GitRepositoryService.branches(@repository.disk_path) + @pull_request = @repository.pull_requests.new(pr_params) + @pull_request.errors.add(:base, + e.is_a?(ActiveRecord::RecordInvalid) ? e.record.errors.full_messages.to_sentence : e.message) + render :new, status: :unprocessable_entity + end + + def show + @comments = @pull_request.comments.includes(:user).order(:created_at) + @commits = GitRepositoryService.commits_between(@repository.disk_path, @pull_request.base_ref, @pull_request.head_ref) + @diff = GitRepositoryService.diff(@repository.disk_path, @pull_request.base_ref, @pull_request.head_ref) + @mergeable = @pull_request.open? && + GitRepositoryService.mergeable?(@repository.disk_path, @pull_request.base_ref, @pull_request.head_ref) + end + + def add_comment + CommentService.create( + repository: @repository, author: current_user, number: @pull_request.number, body: params[:body] + ) + redirect_to repository_pull_request_path(@owner.username, @repository.name, @pull_request.number) + rescue ActiveRecord::RecordInvalid => e + redirect_to repository_pull_request_path(@owner.username, @repository.name, @pull_request.number), + alert: e.record.errors.full_messages.to_sentence + end + + def close + PullRequestService.close(pull_request: @pull_request, actor: current_user) + redirect_to repository_pull_request_path(@owner.username, @repository.name, @pull_request.number), + notice: "Pull request closed." + rescue PullRequestService::Error => e + redirect_to repository_pull_request_path(@owner.username, @repository.name, @pull_request.number), alert: e.message + end + + def reopen + PullRequestService.reopen(pull_request: @pull_request, actor: current_user) + redirect_to repository_pull_request_path(@owner.username, @repository.name, @pull_request.number), + notice: "Pull request reopened." + rescue PullRequestService::Error => e + redirect_to repository_pull_request_path(@owner.username, @repository.name, @pull_request.number), alert: e.message + end + + def merge + PullRequestService.merge(pull_request: @pull_request, merger: current_user) + redirect_to repository_pull_request_path(@owner.username, @repository.name, @pull_request.number), + notice: "Pull request merged." + rescue PullRequestService::Error => e + redirect_to repository_pull_request_path(@owner.username, @repository.name, @pull_request.number), alert: e.message + end + + private + + # PR/comment bodies are free-standing prose, not tied to a file path, so + # they render without the repo/ref/dir context RepositoriesController's + # README rendering resolves relative links against. + def render_markdown(text) + MarkdownRenderer.render(text.to_s).html_safe + end + + def load_pull_request + @pull_request = @repository.pull_requests.find_by!(number: params[:number]) + rescue ActiveRecord::RecordNotFound + render file: Rails.public_path.join("404.html"), status: :not_found, layout: false + end + + def load_owner + @owner = User.find_by!(username: params[:username]) + rescue ActiveRecord::RecordNotFound + render file: Rails.public_path.join("404.html"), status: :not_found, layout: false + end + + def load_repository + @repository = @owner.repositories.find_by!(name: params[:repository]) + rescue ActiveRecord::RecordNotFound + render file: Rails.public_path.join("404.html"), status: :not_found, layout: false + end + + def ensure_can_read! + unless !@repository.is_private || (signed_in? && current_user == @owner) + render file: Rails.public_path.join("404.html"), status: :not_found, layout: false + end + end +end

index 0b6ffb6..5d37e1f 100644 --- a/app/models/pull_request.rb +++ b/app/models/pull_request.rb @@ -25,6 +25,18 @@ class PullRequest < ApplicationRecord user end + def open? + state == "open" + end + + def closed? + state == "closed" + end + + def merged? + state == "merged" + end + def html_url "#{repository.html_url}/pull/#{number}" end

index d60dbcd..c03139e 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -66,6 +66,10 @@ class Repository < ApplicationRecord disk_path end + def open_pull_requests_count + pull_requests.open.count + end + def initialized? return false if git_path.blank? Dir.exist?(git_path) && File.exist?(File.join(git_path, "HEAD"))

index 5b63a8d..fa2c286 100644 --- a/app/services/git_repository_service.rb +++ b/app/services/git_repository_service.rb @@ -256,4 +256,85 @@ class GitRepositoryService return [] unless status.success? out.lines.map(&:strip).reject(&:blank?) end + + # Commits reachable from +head+ but not from +base+ — the commit list a pull + # request shows (same semantics as `git log base..head`). + def self.commits_between(path, base, head, limit: 250) + return [] unless safe_rev?(base) && safe_rev?(head) + format = "%H%x00%h%x00%s%x00%an%x00%ae%x00%ad%x00%cn" + out, _err, status = Open3.capture3( + "git", "--git-dir", path, "log", + "--format=#{format}", "--date=iso-strict", + "-n", limit.to_s, + "#{base}..#{head}" + ) + return [] unless status.success? + out.lines.filter_map do |line| + parts = line.chomp.split("\x00") + next unless parts.length == 7 + sha, short_sha, subject, author_name, author_email, authored_date, committer_name = parts + { + sha: sha, + short_sha: short_sha, + subject: subject, + author_name: author_name, + author_email: author_email, + authored_date: Time.parse(authored_date), + committer_name: committer_name + } + end + end + + # Combined patch for everything +head+ changes relative to +base+, diffed + # against their merge base ("triple-dot" semantics, the same a PR/compare + # view uses) so commits landed on +base+ after the branches diverged don't + # show up as changes. + def self.diff(path, base, head) + return "" unless safe_rev?(base) && safe_rev?(head) + out, _err, status = Open3.capture3("git", "--git-dir", path, "diff", "#{base}...#{head}") + status.success? ? out : "" + end + + # True if +head+ can be merged into +base+ without conflicts. Uses + # `merge-tree --write-tree`, which computes the merge without touching any + # working tree or index — safe to run directly against a bare repo. + def self.mergeable?(path, base, head) + return false unless safe_rev?(base) && safe_rev?(head) + _out, _err, status = Open3.capture3("git", "--git-dir", path, "merge-tree", "--write-tree", base, head) + status.success? + end + + # Merges +head+ into +base+ as a new merge commit (always --no-ff), purely + # via plumbing so no working tree is needed — the repos behind the app are + # bare. Returns the new commit SHA, or nil if either ref is missing, the + # merge has conflicts, or +base+ moved underneath us (a concurrent push, so + # the compare-and-swap ref update was rejected). + def self.merge!(path, base:, head:, message:, author_name:, author_email:) + return nil unless safe_rev?(base) && safe_rev?(head) + + base_sha = commit_sha(path, base) + head_sha = commit_sha(path, head) + return nil unless base_sha && head_sha + + tree_out, _err, tree_status = Open3.capture3( + "git", "--git-dir", path, "merge-tree", "--write-tree", base_sha, head_sha + ) + return nil unless tree_status.success? + tree_sha = tree_out.lines.first.to_s.strip + + env = { + "GIT_AUTHOR_NAME" => author_name, "GIT_AUTHOR_EMAIL" => author_email, + "GIT_COMMITTER_NAME" => author_name, "GIT_COMMITTER_EMAIL" => author_email + } + commit_out, _err, commit_status = Open3.capture3( + env, "git", "--git-dir", path, "commit-tree", tree_sha, "-p", base_sha, "-p", head_sha, "-m", message + ) + return nil unless commit_status.success? + merge_sha = commit_out.strip + + _out, _err, update_status = Open3.capture3( + "git", "--git-dir", path, "update-ref", "refs/heads/#{base}", merge_sha, base_sha + ) + update_status.success? ? merge_sha : nil + end end

index 39fdc0c..2930b06 100644 --- a/app/services/pull_request_service.rb +++ b/app/services/pull_request_service.rb @@ -39,4 +39,55 @@ class PullRequestService repository.issues.maximum(:number) || 0 ].max + 1 end private_class_method :next_number + + # Merges +pull_request+'s head into its base as a new merge commit. Only the + # repo owner may merge (siGit repos have a single owner, so this is the same + # check as opening one). Raises Error if the PR isn't open, a branch has + # disappeared, the head is already merged, or the merge has conflicts. + def self.merge(pull_request:, merger:) + repository = pull_request.repository + unless repository.writable_by?(merger) + raise NotAuthorized, "You don't have permission to merge pull requests on #{repository.full_name}." + end + raise Error, "This pull request is already #{pull_request.state}." unless pull_request.open? + + base_sha = GitRepositoryService.commit_sha(repository.disk_path, pull_request.base_ref) + head_sha = GitRepositoryService.commit_sha(repository.disk_path, pull_request.head_ref) + raise Error, "One of the branches no longer exists." unless base_sha && head_sha + raise Error, "This branch is already up to date with #{pull_request.base_ref}." if base_sha == head_sha + + message = "Merge pull request ##{pull_request.number} from #{pull_request.head_ref}\n\n#{pull_request.title}" + merge_sha = GitRepositoryService.merge!( + repository.disk_path, + base: pull_request.base_ref, head: pull_request.head_ref, message: message, + author_name: merger.username, author_email: merger.email + ) + raise Error, "This pull request has conflicts and can't be merged automatically." unless merge_sha + + pull_request.update!(state: "merged", merged_at: Time.current) + merge_sha + end + + # Closes +pull_request+ without merging. Allowed for the repo owner or the + # PR's own author. + def self.close(pull_request:, actor:) + authorize_modify!(pull_request, actor) + raise Error, "This pull request is already closed." if pull_request.closed? + raise Error, "Merged pull requests can't be closed." if pull_request.merged? + pull_request.update!(state: "closed") + end + + # Reopens a closed (not merged — that's permanent, like GitHub) pull request. + def self.reopen(pull_request:, actor:) + authorize_modify!(pull_request, actor) + raise Error, "Only closed pull requests can be reopened." unless pull_request.closed? + pull_request.update!(state: "open") + end + + def self.authorize_modify!(pull_request, actor) + repository = pull_request.repository + return if repository.writable_by?(actor) || pull_request.user_id == actor&.id + raise NotAuthorized, "You don't have permission to modify this pull request." + end + private_class_method :authorize_modify! end

index b09ac27..1dd2514 100644 --- a/app/views/blobs/show.html.erb +++ b/app/views/blobs/show.html.erb @@ -32,6 +32,16 @@ </svg> Commits <% end %> + <%= link_to repository_pull_requests_path(@owner.username, @repository.name), + class: "tab-item flex items-center gap-1.5" do %> + <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor"> + <path d="M1.5 3.25a2.25 2.25 0 1 1 3 2.122v5.256a2.251 2.251 0 1 1-1.5 0V5.372A2.25 2.25 0 0 1 1.5 3.25Zm5.677-.177L9.573.677A.25.25 0 0 1 10 .854V2.5h1A2.5 2.5 0 0 1 13.5 5v5.628a2.251 2.251 0 1 1-1.5 0V5a1 1 0 0 0-1-1h-1v1.646a.25.25 0 0 1-.427.177L7.177 3.427a.25.25 0 0 1 0-.354ZM3.75 2.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Zm0 9.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5ZM12 12a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Z"/> + </svg> + Pulls + <% if @repository.open_pull_requests_count > 0 %> + <span class="badge-gray text-xs"><%= number_with_delimiter(@repository.open_pull_requests_count) %></span> + <% end %> + <% end %> <%= link_to repository_cicd_path(@owner.username, @repository.name), class: "tab-item flex items-center gap-1.5" do %> <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor">

index f4a35ef..9fec6dd 100644 --- a/app/views/commits/index.html.erb +++ b/app/views/commits/index.html.erb @@ -33,6 +33,16 @@ Commits <span class="badge-gray text-xs"><%= number_with_delimiter(@total) %></span> </div> + <%= link_to repository_pull_requests_path(@owner.username, @repository.name), + class: "tab-item flex items-center gap-1.5" do %> + <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor"> + <path d="M1.5 3.25a2.25 2.25 0 1 1 3 2.122v5.256a2.251 2.251 0 1 1-1.5 0V5.372A2.25 2.25 0 0 1 1.5 3.25Zm5.677-.177L9.573.677A.25.25 0 0 1 10 .854V2.5h1A2.5 2.5 0 0 1 13.5 5v5.628a2.251 2.251 0 1 1-1.5 0V5a1 1 0 0 0-1-1h-1v1.646a.25.25 0 0 1-.427.177L7.177 3.427a.25.25 0 0 1 0-.354ZM3.75 2.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Zm0 9.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5ZM12 12a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Z"/> + </svg> + Pulls + <% if @repository.open_pull_requests_count > 0 %> + <span class="badge-gray text-xs"><%= number_with_delimiter(@repository.open_pull_requests_count) %></span> + <% end %> + <% end %> <%= link_to repository_cicd_path(@owner.username, @repository.name), class: "tab-item flex items-center gap-1.5" do %> <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor">

index 7a321a5..faf4947 100644 --- a/app/views/commits/show.html.erb +++ b/app/views/commits/show.html.erb @@ -30,84 +30,5 @@ </div> <div class="max-w-6xl mx-auto px-4 sm:px-6 py-6 space-y-3"> - <% - file_diffs = [] - current = nil - - @commit[:diff].to_s.each_line do |line| - if line.start_with?("diff --git ") - file_diffs << current if current - current = { header: line, lines: [] } - elsif current - current[:lines] << line - end - end - file_diffs << current if current - %> - - <% if file_diffs.empty? %> - <p class="text-sm text-gray-400">No diff available for this commit.</p> - <% else %> - <% - total_additions = 0 - total_deletions = 0 - file_diffs.each do |fd| - total_additions += fd[:lines].count { |l| l.start_with?("+") && !l.start_with?("+++") } - total_deletions += fd[:lines].count { |l| l.start_with?("-") && !l.start_with?("---") } - end - %> - - <div class="text-xs text-gray-400 mb-4"> - <span class="text-gray-300 font-medium"><%= file_diffs.size %> <%= "file".pluralize(file_diffs.size) %> changed</span> - <% if total_additions > 0 %> - <span class="text-green-400 ml-2">+<%= total_additions %></span> - <% end %> - <% if total_deletions > 0 %> - <span class="text-red-400 ml-1">-<%= total_deletions %></span> - <% end %> - </div> - - <% file_diffs.each do |fd| %> - <% - m = fd[:header].match(/diff --git a\/(.*) b\/(.*)/) - filename = m ? m[2].chomp : fd[:header].chomp - additions = fd[:lines].count { |l| l.start_with?("+") && !l.start_with?("+++") } - deletions = fd[:lines].count { |l| l.start_with?("-") && !l.start_with?("---") } - %> - - <details class="card overflow-hidden group" open> - <summary class="flex items-center justify-between px-4 py-2.5 border-b border-surface-600 bg-surface-700 cursor-pointer list-none hover:bg-surface-600 transition-colors"> - <div class="flex items-center gap-2 min-w-0"> - <svg class="w-3.5 h-3.5 text-gray-500 shrink-0 transition-transform group-open:rotate-90" viewBox="0 0 16 16" fill="currentColor"> - <path d="M6.22 3.22a.75.75 0 0 1 1.06 0l4.25 4.25a.75.75 0 0 1 0 1.06l-4.25 4.25a.75.75 0 0 1-1.06-1.06L9.94 8 6.22 4.28a.75.75 0 0 1 0-1.06Z"/> - </svg> - <span class="font-mono text-xs text-gray-200 truncate"><%= filename %></span> - </div> - <div class="flex items-center gap-2 shrink-0 ml-4 font-mono text-xs"> - <% if additions > 0 %> - <span class="text-green-400">+<%= additions %></span> - <% end %> - <% if deletions > 0 %> - <span class="text-red-400">-<%= deletions %></span> - <% end %> - </div> - </summary> - - <div class="font-mono text-xs leading-5 overflow-x-auto"> - <% fd[:lines].each do |line| %> - <% css = if line.start_with?("+") && !line.start_with?("+++") - "diff-add block px-4" - elsif line.start_with?("-") && !line.start_with?("---") - "diff-remove block px-4" - elsif line.start_with?("@@") - "diff-header block px-4" - else - "block px-4 text-gray-500" - end %> - <span class="<%= css %>"><%= line.chomp %></span> - <% end %> - </div> - </details> - <% end %> - <% end %> + <%= render "shared/diff", diff: @commit[:diff] %> </div>

new file mode 100644 index 0000000..9f5cffb --- /dev/null +++ b/app/views/pull_requests/index.html.erb @@ -0,0 +1,98 @@ +<% content_for :title, "Pull requests · #{@repository.full_name}" %> + +<div class="border-b border-surface-600 bg-surface-800"> + <div class="max-w-6xl mx-auto px-4 sm:px-6 pt-6 pb-0"> + <div class="flex items-center gap-2 text-sm mb-4"> + <svg class="w-4 h-4 text-gray-500" viewBox="0 0 16 16" fill="currentColor"> + <path d="M2 2.5A2.5 2.5 0 0 1 4.5 0h8.75a.75.75 0 0 1 .75.75v12.5a.75.75 0 0 1-.75.75h-2.5a.75.75 0 0 1 0-1.5h1.75v-2h-8a1 1 0 0 0-.714 1.7.75.75 0 1 1-1.072 1.05A2.495 2.495 0 0 1 2 11.5Zm10.5-1h-8a1 1 0 0 0-1 1v6.708A2.486 2.486 0 0 1 4.5 9h8Z"/> + </svg> + <%= link_to "@#{@owner.username}", user_profile_path(@owner.username), class: "text-brand-500 hover:underline font-medium" %> + <span class="text-gray-500">/</span> + <%= link_to @repository.name, repository_path(@owner.username, @repository.name), class: "text-brand-500 hover:underline font-semibold" %> + <% if @repository.is_private %> + <span class="badge-gray ml-1">Private</span> + <% end %> + </div> + + <div class="flex items-center gap-0 -mb-px"> + <%= link_to repository_path(@owner.username, @repository.name), + class: "tab-item flex items-center gap-1.5" do %> + <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor"> + <path d="M2 2.5A2.5 2.5 0 0 1 4.5 0h8.75a.75.75 0 0 1 .75.75v12.5a.75.75 0 0 1-.75.75h-2.5a.75.75 0 0 1 0-1.5h1.75v-2h-8a1 1 0 0 0-.714 1.7.75.75 0 1 1-1.072 1.05A2.495 2.495 0 0 1 2 11.5Zm10.5-1h-8a1 1 0 0 0-1 1v6.708A2.486 2.486 0 0 1 4.5 9h8Z"/> + </svg> + Code + <% end %> + <% if @repository.initialized? %> + <%= link_to repository_commits_path(@owner.username, @repository.name, @repository.default_branch), + class: "tab-item flex items-center gap-1.5" do %> + <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor"> + <path d="M11.93 8.5a4.002 4.002 0 0 1-7.86 0H.75a.75.75 0 0 1 0-1.5h3.32a4.002 4.002 0 0 1 7.86 0h3.32a.75.75 0 0 1 0 1.5Zm-1.43-.75a2.5 2.5 0 1 0-5 0 2.5 2.5 0 0 0 5 0Z"/> + </svg> + Commits + <% end %> + <% end %> + <div class="tab-item active flex items-center gap-1.5"> + <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor"> + <path d="M1.5 3.25a2.25 2.25 0 1 1 3 2.122v5.256a2.251 2.251 0 1 1-1.5 0V5.372A2.25 2.25 0 0 1 1.5 3.25Zm5.677-.177L9.573.677A.25.25 0 0 1 10 .854V2.5h1A2.5 2.5 0 0 1 13.5 5v5.628a2.251 2.251 0 1 1-1.5 0V5a1 1 0 0 0-1-1h-1v1.646a.25.25 0 0 1-.427.177L7.177 3.427a.25.25 0 0 1 0-.354ZM3.75 2.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Zm0 9.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5ZM12 12a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Z"/> + </svg> + Pulls + <% if @repository.open_pull_requests_count > 0 %> + <span class="badge-gray text-xs"><%= number_with_delimiter(@repository.open_pull_requests_count) %></span> + <% end %> + </div> + <%= link_to repository_cicd_path(@owner.username, @repository.name), + class: "tab-item flex items-center gap-1.5" do %> + <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor"> + <path d="M4.75 4.5C2.679 4.5 1 5.958 1 7.75S2.679 11 4.75 11c1.028 0 1.948-.36 2.75-1.242l.652-.716.696.672C10.023 10.849 10.956 11 11.25 11 13.321 11 15 9.542 15 7.75S13.321 4.5 11.25 4.5c-1.028 0-1.948.36-2.75 1.242l-.652.716-.696-.672C5.977 4.651 5.044 4.5 4.75 4.5Zm0 1.5c.483 0 .943.152 1.36.556l.562.541-.562.618C5.693 8.119 5.233 8.5 4.75 8.5c-1.241 0-2.25-.672-2.25-1.5S3.509 6 4.75 6Zm6.5 0c1.241 0 2.25.672 2.25 1.5S12.491 9 11.25 9c-.483 0-.943-.152-1.36-.556l-.562-.541.562-.618c.417-.404.877-.785 1.36-.785Z"/> + </svg> + CI/CD + <% end %> + </div> + </div> +</div> + +<div class="max-w-6xl mx-auto px-4 sm:px-6 py-6"> + <div class="flex items-center justify-between mb-4"> + <div class="flex items-center gap-1 text-sm"> + <% [["open", "Open", @open_count], ["closed", "Closed", @closed_count], ["merged", "Merged", @merged_count], ["all", "All", nil]].each do |state, label, count| %> + <%= link_to repository_pull_requests_path(@owner.username, @repository.name, state: state), + class: "px-3 py-1.5 rounded font-medium #{@state == state ? 'bg-surface-600 text-gray-100' : 'text-gray-400 hover:text-gray-200'}" do %> + <%= label %><% if count %> <span class="text-xs text-gray-500">(<%= count %>)</span><% end %> + <% end %> + <% end %> + </div> + <% if @repository.writable_by?(current_user) %> + <%= link_to "New pull request", new_repository_pull_request_path(@owner.username, @repository.name), class: "btn-primary" %> + <% end %> + </div> + + <div class="card"> + <% @pull_requests.each do |pr| %> + <div class="commit-row"> + <div class="min-w-0 flex-1"> + <p class="text-sm font-medium text-gray-100 truncate"> + <%= link_to pr.title, repository_pull_request_path(@owner.username, @repository.name, pr.number), class: "hover:text-brand-500" %> + </p> + <p class="text-xs text-gray-400 mt-0.5"> + #<%= pr.number %> opened by <span class="font-medium"><%= pr.author.username %></span> + &middot; <span class="font-mono"><%= pr.head_ref %></span> &rarr; <span class="font-mono"><%= pr.base_ref %></span> + </p> + </div> + <div class="shrink-0 ml-4"> + <% case pr.state + when "open" %> + <span class="badge-green">Open</span> + <% when "merged" %> + <span class="badge-purple">Merged</span> + <% else %> + <span class="badge-red">Closed</span> + <% end %> + </div> + </div> + <% end %> + + <% if @pull_requests.empty? %> + <div class="px-4 py-12 text-center text-sm text-gray-400">No pull requests here.</div> + <% end %> + </div> +</div>

new file mode 100644 index 0000000..87515db --- /dev/null +++ b/app/views/pull_requests/new.html.erb @@ -0,0 +1,55 @@ +<% content_for :title, "New pull request · #{@repository.full_name}" %> + +<div class="max-w-2xl mx-auto px-4 sm:px-6 py-12"> + <div class="flex items-center gap-2 text-sm mb-2"> + <%= link_to "@#{@owner.username}", user_profile_path(@owner.username), class: "text-brand-500 hover:underline font-medium" %> + <span class="text-gray-500">/</span> + <%= link_to @repository.name, repository_path(@owner.username, @repository.name), class: "text-brand-500 hover:underline font-semibold" %> + <span class="text-gray-500">/</span> + <%= link_to "pulls", repository_pull_requests_path(@owner.username, @repository.name), class: "text-gray-400 hover:text-gray-200 transition-colors" %> + </div> + <h1 class="text-xl font-semibold text-gray-100 mb-2">New pull request</h1> + <p class="text-sm text-gray-400 mb-8">Propose merging one branch into another in this repository.</p> + + <% if @pull_request.errors.any? %> + <div class="border border-red-800/40 bg-red-900/20 rounded px-4 py-3 mb-6"> + <p class="text-sm font-medium text-red-300 mb-1">Please fix the following errors:</p> + <ul class="list-disc pl-4 space-y-0.5"> + <% @pull_request.errors.full_messages.each do |msg| %> + <li class="text-sm text-red-400"><%= msg %></li> + <% end %> + </ul> + </div> + <% end %> + + <%= form_with model: @pull_request, url: repository_pull_requests_path(@owner.username, @repository.name), + method: :post, class: "space-y-6" do |f| %> + <div class="flex items-end gap-2"> + <div class="flex-1 min-w-0"> + <label class="form-label">base</label> + <%= f.select :base_ref, @branches, { selected: @pull_request.base_ref }, class: "form-input font-mono" %> + </div> + <div class="flex items-center pb-2.5 text-gray-500">&larr;</div> + <div class="flex-1 min-w-0"> + <label class="form-label">compare</label> + <%= f.select :head_ref, @branches, { selected: @pull_request.head_ref }, class: "form-input font-mono" %> + </div> + </div> + + <div> + <%= f.label :title, class: "form-label" %> + <%= f.text_field :title, class: "form-input", placeholder: "Short summary of the change", autofocus: true %> + </div> + + <div> + <%= f.label :body, "Description", class: "form-label" %> + <span class="text-xs text-gray-500 ml-1">(optional, Markdown)</span> + <%= f.text_area :body, class: "form-input", rows: 6, placeholder: "What does this change do, and why?" %> + </div> + + <div class="pt-2 border-t border-surface-600 flex items-center gap-3"> + <%= f.submit "Create pull request", class: "btn-primary cursor-pointer" %> + <%= link_to "Cancel", repository_pull_requests_path(@owner.username, @repository.name), class: "btn-ghost" %> + </div> + <% end %> +</div>

new file mode 100644 index 0000000..396cec9 --- /dev/null +++ b/app/views/pull_requests/show.html.erb @@ -0,0 +1,134 @@ +<% content_for :title, "#{@pull_request.title} (##{@pull_request.number}) · #{@repository.full_name}" %> + +<div class="border-b border-surface-600 bg-surface-800"> + <div class="max-w-4xl mx-auto px-4 sm:px-6 pt-6 pb-4"> + <div class="flex items-center gap-2 text-sm mb-4"> + <%= link_to "@#{@owner.username}", user_profile_path(@owner.username), class: "text-brand-500 hover:underline font-medium" %> + <span class="text-gray-500">/</span> + <%= link_to @repository.name, repository_path(@owner.username, @repository.name), class: "text-brand-500 hover:underline font-semibold" %> + <span class="text-gray-500">/</span> + <%= link_to "pulls", repository_pull_requests_path(@owner.username, @repository.name), class: "text-gray-400 hover:text-gray-200 transition-colors" %> + <span class="text-gray-500">/</span> + <span class="text-gray-400">#<%= @pull_request.number %></span> + </div> + + <h1 class="text-xl font-semibold text-gray-100 mb-2"><%= @pull_request.title %> + <span class="text-gray-500 font-normal">#<%= @pull_request.number %></span> + </h1> + + <div class="flex flex-wrap items-center gap-2 text-sm"> + <% case @pull_request.state + when "open" %> + <span class="badge-green">Open</span> + <% when "merged" %> + <span class="badge-purple">Merged</span> + <% else %> + <span class="badge-red">Closed</span> + <% end %> + <span class="text-gray-400"> + <span class="font-medium text-gray-300"><%= @pull_request.author.username %></span> wants to merge + <span class="font-mono badge-gray"><%= @pull_request.head_ref %></span> + into + <span class="font-mono badge-gray"><%= @pull_request.base_ref %></span> + </span> + </div> + </div> +</div> + +<div class="max-w-4xl mx-auto px-4 sm:px-6 py-6 space-y-6"> + <% if @pull_request.body.present? %> + <div class="card"> + <div class="px-6 py-6 prose-readme"><%= render_markdown(@pull_request.body) %></div> + </div> + <% end %> + + <% if @pull_request.open? && @repository.writable_by?(current_user) %> + <div class="card px-4 py-3 flex items-center justify-between gap-3 + <%= @mergeable ? 'bg-green-900/10 border-green-800/30' : 'bg-amber-900/10 border-amber-800/30' %>"> + <% if @mergeable %> + <p class="text-sm text-green-300">This branch has no conflicts with the base branch.</p> + <%= button_to "Merge pull request", merge_repository_pull_request_path(@owner.username, @repository.name, @pull_request.number), + method: :post, class: "btn-primary cursor-pointer", + form: { data: { turbo_confirm: "Merge ##{@pull_request.number} into #{@pull_request.base_ref}?" } } %> + <% else %> + <p class="text-sm text-amber-300">This branch has conflicts and can't be merged automatically.</p> + <% end %> + </div> + <% end %> + + <div> + <h2 class="text-sm font-medium text-gray-300 mb-2"> + <%= pluralize(@commits.size, "commit") %> + </h2> + <div class="card"> + <% @commits.each do |commit| %> + <div class="commit-row"> + <div class="min-w-0 flex-1"> + <p class="text-sm font-medium text-gray-100 truncate"> + <%= link_to commit[:subject], repository_commit_path(@owner.username, @repository.name, commit[:sha]), class: "hover:text-brand-500" %> + </p> + <p class="text-xs text-gray-400 mt-0.5"> + <span class="font-medium"><%= commit[:author_name] %></span> committed <%= time_ago_in_words(commit[:authored_date]) %> ago + </p> + </div> + <div class="shrink-0 ml-4"> + <%= link_to commit[:short_sha], repository_commit_path(@owner.username, @repository.name, commit[:sha]), + class: "font-mono text-xs badge-gray hover:bg-surface-500" %> + </div> + </div> + <% end %> + <% if @commits.empty? %> + <div class="px-4 py-6 text-center text-sm text-gray-400">No commits.</div> + <% end %> + </div> + </div> + + <div> + <h2 class="text-sm font-medium text-gray-300 mb-2">Files changed</h2> + <%= render "shared/diff", diff: @diff %> + </div> + + <div> + <h2 class="text-sm font-medium text-gray-300 mb-2"> + <%= pluralize(@comments.size, "comment") %> + </h2> + <div class="space-y-3"> + <% @comments.each do |comment| %> + <div class="card" id="comment-<%= comment.id %>"> + <div class="px-4 py-2 border-b border-surface-600 bg-surface-700 text-xs text-gray-400"> + <span class="font-medium text-gray-200"><%= comment.author.username %></span> + commented <%= time_ago_in_words(comment.created_at) %> ago + </div> + <div class="px-6 py-4 prose-readme"><%= render_markdown(comment.body) %></div> + </div> + <% end %> + </div> + + <% if signed_in? %> + <%= form_with url: repository_pull_request_comments_path(@owner.username, @repository.name, @pull_request.number), + method: :post, class: "mt-4 space-y-2" do |f| %> + <%= f.label :body, "Add a comment", class: "form-label" %> + <%= f.text_area :body, class: "form-input", rows: 4, placeholder: "Leave a comment", required: true %> + <div class="flex justify-end"> + <%= f.submit "Comment", class: "btn-primary cursor-pointer" %> + </div> + <% end %> + <% else %> + <p class="text-sm text-gray-400 mt-4"> + <%= link_to "Sign in", signin_path, class: "text-brand-500 hover:underline" %> to leave a comment. + </p> + <% end %> + </div> + + <% if @repository.writable_by?(current_user) || @pull_request.user_id == current_user&.id %> + <div class="pt-2 border-t border-surface-600 flex items-center gap-3"> + <% if @pull_request.open? %> + <%= button_to "Close pull request", close_repository_pull_request_path(@owner.username, @repository.name, @pull_request.number), + method: :post, class: "btn-secondary cursor-pointer" %> + <% elsif @pull_request.closed? %> + <%= button_to "Reopen pull request", reopen_repository_pull_request_path(@owner.username, @repository.name, @pull_request.number), + method: :post, class: "btn-secondary cursor-pointer" %> + <% end %> + </div> + <% end %> +</div>

index d48f302..00b5f4d 100644 --- a/app/views/repositories/cicd.html.erb +++ b/app/views/repositories/cicd.html.erb @@ -41,6 +41,16 @@ <% end %> <% end %> <% end %> + <%= link_to repository_pull_requests_path(@owner.username, @repository.name), + class: "tab-item flex items-center gap-1.5" do %> + <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor"> + <path d="M1.5 3.25a2.25 2.25 0 1 1 3 2.122v5.256a2.251 2.251 0 1 1-1.5 0V5.372A2.25 2.25 0 0 1 1.5 3.25Zm5.677-.177L9.573.677A.25.25 0 0 1 10 .854V2.5h1A2.5 2.5 0 0 1 13.5 5v5.628a2.251 2.251 0 1 1-1.5 0V5a1 1 0 0 0-1-1h-1v1.646a.25.25 0 0 1-.427.177L7.177 3.427a.25.25 0 0 1 0-.354ZM3.75 2.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Zm0 9.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5ZM12 12a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Z"/> + </svg> + Pulls + <% if @repository.open_pull_requests_count > 0 %> + <span class="badge-gray text-xs"><%= number_with_delimiter(@repository.open_pull_requests_count) %></span> + <% end %> + <% end %> <div class="tab-item active flex items-center gap-1.5"> <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor"> <path d="M4.75 4.5C2.679 4.5 1 5.958 1 7.75S2.679 11 4.75 11c1.028 0 1.948-.36 2.75-1.242l.652-.716.696.672C10.023 10.849 10.956 11 11.25 11 13.321 11 15 9.542 15 7.75S13.321 4.5 11.25 4.5c-1.028 0-1.948.36-2.75 1.242l-.652.716-.696-.672C5.977 4.651 5.044 4.5 4.75 4.5Zm0 1.5c.483 0 .943.152 1.36.556l.562.541-.562.618C5.693 8.119 5.233 8.5 4.75 8.5c-1.241 0-2.25-.672-2.25-1.5S3.509 6 4.75 6Zm6.5 0c1.241 0 2.25.672 2.25 1.5S12.491 9 11.25 9c-.483 0-.943-.152-1.36-.556l-.562-.541.562-.618c.417-.404.877-.785 1.36-.785Z"/>

index d94d36b..a573e68 100644 --- a/app/views/repositories/show.html.erb +++ b/app/views/repositories/show.html.erb @@ -44,6 +44,16 @@ <% end %> <% end %> <% end %> + <%= link_to repository_pull_requests_path(@owner.username, @repository.name), + class: "tab-item flex items-center gap-1.5" do %> + <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor"> + <path d="M1.5 3.25a2.25 2.25 0 1 1 3 2.122v5.256a2.251 2.251 0 1 1-1.5 0V5.372A2.25 2.25 0 0 1 1.5 3.25Zm5.677-.177L9.573.677A.25.25 0 0 1 10 .854V2.5h1A2.5 2.5 0 0 1 13.5 5v5.628a2.251 2.251 0 1 1-1.5 0V5a1 1 0 0 0-1-1h-1v1.646a.25.25 0 0 1-.427.177L7.177 3.427a.25.25 0 0 1 0-.354ZM3.75 2.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Zm0 9.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5ZM12 12a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Z"/> + </svg> + Pulls + <% if @repository.open_pull_requests_count > 0 %> + <span class="badge-gray text-xs"><%= number_with_delimiter(@repository.open_pull_requests_count) %></span> + <% end %> + <% end %> <%= link_to repository_cicd_path(@owner.username, @repository.name), class: "tab-item flex items-center gap-1.5" do %> <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor">

index 6cae93b..451e322 100644 --- a/app/views/repositories/tree.html.erb +++ b/app/views/repositories/tree.html.erb @@ -44,6 +44,16 @@ </svg> Commits <% end %> + <%= link_to repository_pull_requests_path(@owner.username, @repository.name), + class: "tab-item flex items-center gap-1.5" do %> + <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor"> + <path d="M1.5 3.25a2.25 2.25 0 1 1 3 2.122v5.256a2.251 2.251 0 1 1-1.5 0V5.372A2.25 2.25 0 0 1 1.5 3.25Zm5.677-.177L9.573.677A.25.25 0 0 1 10 .854V2.5h1A2.5 2.5 0 0 1 13.5 5v5.628a2.251 2.251 0 1 1-1.5 0V5a1 1 0 0 0-1-1h-1v1.646a.25.25 0 0 1-.427.177L7.177 3.427a.25.25 0 0 1 0-.354ZM3.75 2.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Zm0 9.5a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5ZM12 12a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Z"/> + </svg> + Pulls + <% if @repository.open_pull_requests_count > 0 %> + <span class="badge-gray text-xs"><%= number_with_delimiter(@repository.open_pull_requests_count) %></span> + <% end %> + <% end %> <%= link_to repository_cicd_path(@owner.username, @repository.name), class: "tab-item flex items-center gap-1.5" do %> <svg class="w-4 h-4" viewBox="0 0 16 16" fill="currentColor">

new file mode 100644 index 0000000..753f4be --- /dev/null +++ b/app/views/shared/_diff.html.erb @@ -0,0 +1,83 @@ +<%# locals: diff (raw unified-diff text) %> +<% + file_diffs = [] + current = nil + + diff.to_s.each_line do |line| + if line.start_with?("diff --git ") + file_diffs << current if current + current = { header: line, lines: [] } + elsif current + current[:lines] << line + end + end + file_diffs << current if current +%> + +<% if file_diffs.empty? %> + <p class="text-sm text-gray-400">No changes.</p> +<% else %> + <% + total_additions = 0 + total_deletions = 0 + file_diffs.each do |fd| + total_additions += fd[:lines].count { |l| l.start_with?("+") && !l.start_with?("+++") } + total_deletions += fd[:lines].count { |l| l.start_with?("-") && !l.start_with?("---") } + end + %> + + <div class="text-xs text-gray-400 mb-4"> + <span class="text-gray-300 font-medium"><%= file_diffs.size %> <%= "file".pluralize(file_diffs.size) %> changed</span> + <% if total_additions > 0 %> + <span class="text-green-400 ml-2">+<%= total_additions %></span> + <% end %> + <% if total_deletions > 0 %> + <span class="text-red-400 ml-1">-<%= total_deletions %></span> + <% end %> + </div> + + <div class="space-y-3"> + <% file_diffs.each do |fd| %> + <% + m = fd[:header].match(/diff --git a\/(.*) b\/(.*)/) + filename = m ? m[2].chomp : fd[:header].chomp + additions = fd[:lines].count { |l| l.start_with?("+") && !l.start_with?("+++") } + deletions = fd[:lines].count { |l| l.start_with?("-") && !l.start_with?("---") } + %> + + <details class="card overflow-hidden group" open> + <summary class="flex items-center justify-between px-4 py-2.5 border-b border-surface-600 bg-surface-700 cursor-pointer list-none hover:bg-surface-600 transition-colors"> + <div class="flex items-center gap-2 min-w-0"> + <svg class="w-3.5 h-3.5 text-gray-500 shrink-0 transition-transform group-open:rotate-90" viewBox="0 0 16 16" fill="currentColor"> + <path d="M6.22 3.22a.75.75 0 0 1 1.06 0l4.25 4.25a.75.75 0 0 1 0 1.06l-4.25 4.25a.75.75 0 0 1-1.06-1.06L9.94 8 6.22 4.28a.75.75 0 0 1 0-1.06Z"/> + </svg> + <span class="font-mono text-xs text-gray-200 truncate"><%= filename %></span> + </div> + <div class="flex items-center gap-2 shrink-0 ml-4 font-mono text-xs"> + <% if additions > 0 %> + <span class="text-green-400">+<%= additions %></span> + <% end %> + <% if deletions > 0 %> + <span class="text-red-400">-<%= deletions %></span> + <% end %> + </div> + </summary> + + <div class="font-mono text-xs leading-5 overflow-x-auto"> + <% fd[:lines].each do |line| %> + <% css = if line.start_with?("+") && !line.start_with?("+++") + "diff-add block px-4" + elsif line.start_with?("-") && !line.start_with?("---") + "diff-remove block px-4" + elsif line.start_with?("@@") + "diff-header block px-4" + else + "block px-4 text-gray-500" + end %> + <span class="<%= css %>"><%= line.chomp %></span> + <% end %> + </div> + </details> + <% end %> + </div> +<% end %>

index add094f..7902dfd 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -155,6 +155,22 @@ Rails.application.routes.draw do constraints: { branch: /[^\/]+/, repository: /[^\/.][^\/]*/ } get "/:username/:repository/ci-cd", to: "repositories#cicd", as: :repository_cicd get "/:username/:repository/commit/:sha", to: "commits#show", as: :repository_commit + + # Pull requests — creation-only backend already existed (PullRequestService, + # exposed over MCP); these add the web surface on top of it. + get "/:username/:repository/pulls", to: "pull_requests#index", as: :repository_pull_requests + get "/:username/:repository/pulls/new", to: "pull_requests#new", as: :new_repository_pull_request + post "/:username/:repository/pulls", to: "pull_requests#create" + get "/:username/:repository/pull/:number", to: "pull_requests#show", as: :repository_pull_request, + constraints: { number: /\d+/ } + post "/:username/:repository/pull/:number/comments", to: "pull_requests#add_comment", + as: :repository_pull_request_comments, constraints: { number: /\d+/ } + post "/:username/:repository/pull/:number/close", to: "pull_requests#close", as: :close_repository_pull_request, + constraints: { number: /\d+/ } + post "/:username/:repository/pull/:number/reopen", to: "pull_requests#reopen", as: :reopen_repository_pull_request, + constraints: { number: /\d+/ } + post "/:username/:repository/pull/:number/merge", to: "pull_requests#merge", as: :merge_repository_pull_request, + constraints: { number: /\d+/ } get "/:username/:repository/blob/:branch/*path", to: "blobs#show", as: :repository_blob, format: false get "/:username/:repository/raw/:branch/*path", to: "blobs#raw", as: :repository_blob_raw, format: false get "/:username/:repository/tree/:branch/*path", to: "repositories#tree", as: :repository_tree

new file mode 100644 index 0000000..025e53f --- /dev/null +++ b/spec/requests/pull_requests_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true + +require "rails_helper" +require "tmpdir" +require "fileutils" + +RSpec.describe "Pull requests", type: :request do + around do |example| + Dir.mktmpdir("pull-requests-spec") do |tmp| + @repo_dir = File.join(tmp, "demo.git") + build_repo(@repo_dir) + example.run + end + end + + let(:owner) { User.create!(smbcloud_id: 9001, email: "pr-owner@example.com", username: "prowner") } + let(:outsider) { User.create!(smbcloud_id: 9002, email: "pr-outsider@example.com", username: "proutsider") } + let!(:repo) do + owner.repositories.create!(name: "demo", kind: "code", default_branch: "main", disk_path: @repo_dir) + end + + # Sign-in runs through smbCloud's native gem in production, so request specs + # stub current_user rather than log in for real (same pattern as admin_spec.rb). + def sign_in(user) + allow_any_instance_of(ApplicationController).to receive(:current_user).and_return(user) + end + + describe "GET /:username/:repository/pulls" do + it "lists open pull requests by default" do + repo.pull_requests.create!(user: owner, number: 1, title: "Add feature", head_ref: "feature", base_ref: "main") + + get "/prowner/demo/pulls" + + expect(response).to have_http_status(:success) + expect(response.body).to include("Add feature") + end + + it "is reachable without signing in for a public repo" do + get "/prowner/demo/pulls" + expect(response).to have_http_status(:success) + end + end + + describe "GET /:username/:repository/pull/:number" do + it "shows the PR with its commits, diff, and comments" do + pr = repo.pull_requests.create!(user: owner, number: 1, title: "Add feature", body: "Why this matters", head_ref: "feature", base_ref: "main") + pr.comments.create!(user: owner, body: "Looks good") + + get "/prowner/demo/pull/1" + + expect(response).to have_http_status(:success) + expect(response.body).to include("Add feature") + expect(response.body).to include("feature.txt") + expect(response.body).to include("Looks good") + end + end + + describe "POST /:username/:repository/pulls" do + it "lets the owner open a pull request" do + sign_in(owner) + + post "/prowner/demo/pulls", params: { pull_request: { title: "Add feature", head_ref: "feature", base_ref: "main" } } + + expect(response).to redirect_to("/prowner/demo/pull/1") + expect(repo.pull_requests.count).to eq(1) + end + + it "refuses a non-owner" do + sign_in(outsider) + + post "/prowner/demo/pulls", params: { pull_request: { title: "Add feature", head_ref: "feature", base_ref: "main" } } + + expect(repo.pull_requests.count).to eq(0) + end + end + + describe "POST /:username/:repository/pull/:number/merge" do + it "merges a mergeable PR for the owner" do + pr = repo.pull_requests.create!(user: owner, number: 1, title: "Add feature", head_ref: "feature", base_ref: "main") + sign_in(owner) + + post "/prowner/demo/pull/1/merge" + + expect(response).to redirect_to("/prowner/demo/pull/1") + expect(pr.reload.state).to eq("merged") + end + + it "refuses a non-owner" do + pr = repo.pull_requests.create!(user: owner, number: 1, title: "Add feature", head_ref: "feature", base_ref: "main") + sign_in(outsider) + + post "/prowner/demo/pull/1/merge" + + expect(pr.reload.state).to eq("open") + end + end + + describe "POST /:username/:repository/pull/:number/close and /reopen" do + it "closes then reopens a PR for the owner" do + pr = repo.pull_requests.create!(user: owner, number: 1, title: "Add feature", head_ref: "feature", base_ref: "main") + sign_in(owner) + + post "/prowner/demo/pull/1/close" + expect(pr.reload.state).to eq("closed") + + post "/prowner/demo/pull/1/reopen" + expect(pr.reload.state).to eq("open") + end + end + + describe "POST /:username/:repository/pull/:number/comments" do + it "lets a signed-in reader comment" do + repo.pull_requests.create!(user: owner, number: 1, title: "Add feature", head_ref: "feature", base_ref: "main") + sign_in(outsider) + + post "/prowner/demo/pull/1/comments", params: { body: "Nice work" } + + expect(response).to redirect_to("/prowner/demo/pull/1") + expect(Comment.last.body).to eq("Nice work") + end + end + + def build_repo(bare_path) + FileUtils.mkdir_p(bare_path) + system("git", "init", "--bare", bare_path, exception: true) + Dir.mktmpdir do |work| + system("git", "-C", work, "init", "-b", "main", exception: true) + system("git", "-C", work, "config", "user.email", "t@example.com", exception: true) + system("git", "-C", work, "config", "user.name", "Test", exception: true) + File.write(File.join(work, "README.md"), "# Demo\n") + system("git", "-C", work, "add", ".", exception: true) + system("git", "-C", work, "commit", "-m", "seed", exception: true) + system("git", "-C", work, "remote", "add", "origin", bare_path, exception: true) + system("git", "-C", work, "push", "origin", "main", exception: true) + + system("git", "-C", work, "checkout", "-b", "feature", exception: true) + File.write(File.join(work, "feature.txt"), "hi\n") + system("git", "-C", work, "add", ".", exception: true) + system("git", "-C", work, "commit", "-m", "feature commit", exception: true) + system("git", "-C", work, "push", "origin", "feature", exception: true) + end + end +end

new file mode 100644 index 0000000..7ab6744 --- /dev/null +++ b/spec/services/git_repository_service_merge_spec.rb @@ -0,0 +1,122 @@ +# frozen_string_literal: true + +require "rails_helper" +require "tmpdir" +require "fileutils" + +# Covers the git plumbing that backs pull request merging: comparing two +# branches and creating a merge commit without a working tree (the app's +# repos are bare). +RSpec.describe GitRepositoryService do + around do |example| + Dir.mktmpdir("git-merge-spec") do |tmp| + @repo_dir = File.join(tmp, "demo.git") + build_repo(@repo_dir) + example.run + end + end + + describe ".commits_between" do + it "lists commits reachable from head but not base" do + commits = described_class.commits_between(@repo_dir, "main", "feature") + expect(commits.map { |c| c[:subject] }).to eq([ "feature commit" ]) + end + + it "returns nothing once the branches are equal" do + expect(described_class.commits_between(@repo_dir, "main", "main")).to eq([]) + end + end + + describe ".diff" do + it "includes the file added on the feature branch" do + diff = described_class.diff(@repo_dir, "main", "feature") + expect(diff).to include("feature.txt") + end + end + + describe ".mergeable?" do + it "is true for a clean merge" do + expect(described_class.mergeable?(@repo_dir, "main", "feature")).to be(true) + end + + it "is false when both branches touch the same lines" do + expect(described_class.mergeable?(@repo_dir, "conflict-a", "conflict-b")).to be(false) + end + end + + describe ".merge!" do + it "creates a merge commit and advances base" do + before_sha = described_class.commit_sha(@repo_dir, "main") + sha = described_class.merge!( + @repo_dir, base: "main", head: "feature", message: "Merge feature", + author_name: "Test Merger", author_email: "merger@example.com" + ) + + expect(sha).to be_present + expect(described_class.commit_sha(@repo_dir, "main")).to eq(sha) + expect(described_class.commit_sha(@repo_dir, "main")).not_to eq(before_sha) + + commit = described_class.commit(@repo_dir, sha) + expect(commit[:author_name]).to eq("Test Merger") + expect(commit[:author_email]).to eq("merger@example.com") + + # Feature's file is now reachable from main. + expect(described_class.file_content(@repo_dir, "main", "feature.txt")).to eq("hi\n") + end + + it "returns nil and leaves base untouched on conflict" do + before_sha = described_class.commit_sha(@repo_dir, "conflict-a") + sha = described_class.merge!( + @repo_dir, base: "conflict-a", head: "conflict-b", message: "Merge", + author_name: "Test", author_email: "test@example.com" + ) + + expect(sha).to be_nil + expect(described_class.commit_sha(@repo_dir, "conflict-a")).to eq(before_sha) + end + + it "returns nil for a missing branch" do + sha = described_class.merge!( + @repo_dir, base: "main", head: "does-not-exist", message: "Merge", + author_name: "Test", author_email: "test@example.com" + ) + expect(sha).to be_nil + end + end + + def build_repo(bare_path) + FileUtils.mkdir_p(bare_path) + system("git", "init", "--bare", bare_path, exception: true) + Dir.mktmpdir do |work| + system("git", "-C", work, "init", "-b", "main", exception: true) + system("git", "-C", work, "config", "user.email", "t@example.com", exception: true) + system("git", "-C", work, "config", "user.name", "Test", exception: true) + File.write(File.join(work, "README.md"), "# Demo\n") + system("git", "-C", work, "add", ".", exception: true) + system("git", "-C", work, "commit", "-m", "seed", exception: true) + system("git", "-C", work, "remote", "add", "origin", bare_path, exception: true) + system("git", "-C", work, "push", "origin", "main", exception: true) + + system("git", "-C", work, "checkout", "-b", "feature", exception: true) + File.write(File.join(work, "feature.txt"), "hi\n") + system("git", "-C", work, "add", ".", exception: true) + system("git", "-C", work, "commit", "-m", "feature commit", exception: true) + system("git", "-C", work, "push", "origin", "feature", exception: true) + + # Two branches that both edit line 1 of the same file — a genuine conflict. + system("git", "-C", work, "checkout", "main", exception: true) + system("git", "-C", work, "checkout", "-b", "conflict-a", exception: true) + File.write(File.join(work, "shared.txt"), "a\n") + system("git", "-C", work, "add", ".", exception: true) + system("git", "-C", work, "commit", "-m", "conflict a", exception: true) + system("git", "-C", work, "push", "origin", "conflict-a", exception: true) + + system("git", "-C", work, "checkout", "main", exception: true) + system("git", "-C", work, "checkout", "-b", "conflict-b", exception: true) + File.write(File.join(work, "shared.txt"), "b\n") + system("git", "-C", work, "add", ".", exception: true) + system("git", "-C", work, "commit", "-m", "conflict b", exception: true) + system("git", "-C", work, "push", "origin", "conflict-b", exception: true) + end + end +end

index 98c9ca3..0af4527 100644 --- a/spec/services/pull_request_service_spec.rb +++ b/spec/services/pull_request_service_spec.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true require "rails_helper" +require "tmpdir" +require "fileutils" RSpec.describe PullRequestService do let(:owner) { User.create!(smbcloud_id: 7001, email: "owner@example.com", username: "owner") } @@ -55,4 +57,135 @@ RSpec.describe PullRequestService do pr = described_class.create(repository: repo, author: owner, title: "PR", head: "feature", base: "main") expect(pr.number).to eq(2) end + + # Merge/close/reopen exercise real git plumbing, so these need an actual + # repo on disk rather than the stubbed branch_exists? above. + describe "merge/close/reopen" do + around do |example| + Dir.mktmpdir("pr-service-spec") do |tmp| + @repo_dir = File.join(tmp, "app.git") + build_repo(@repo_dir) + example.run + end + end + + let(:repo_on_disk) do + owner.repositories.create!(name: "app", kind: "code", default_branch: "main", disk_path: @repo_dir) + end + + it "merges an open PR and marks it merged" do + pr = repo_on_disk.pull_requests.create!(user: owner, number: 1, title: "Add feature", head_ref: "feature", base_ref: "main") + + sha = described_class.merge(pull_request: pr, merger: owner) + + expect(sha).to be_present + expect(pr.reload.state).to eq("merged") + expect(pr.merged_at).to be_present + expect(GitRepositoryService.commit_sha(@repo_dir, "main")).to eq(sha) + end + + it "refuses to merge for a non-owner" do + pr = repo_on_disk.pull_requests.create!(user: owner, number: 1, title: "X", head_ref: "feature", base_ref: "main") + + expect do + described_class.merge(pull_request: pr, merger: outsider) + end.to raise_error(PullRequestService::NotAuthorized) + expect(pr.reload.state).to eq("open") + end + + it "refuses to merge a conflicting branch" do + pr = repo_on_disk.pull_requests.create!(user: owner, number: 1, title: "X", head_ref: "conflict-b", base_ref: "conflict-a") + + expect do + described_class.merge(pull_request: pr, merger: owner) + end.to raise_error(PullRequestService::Error, /conflicts/) + expect(pr.reload.state).to eq("open") + end + + it "refuses to merge a PR that isn't open" do + pr = repo_on_disk.pull_requests.create!(user: owner, number: 1, title: "X", head_ref: "feature", base_ref: "main", state: "closed") + + expect do + described_class.merge(pull_request: pr, merger: owner) + end.to raise_error(PullRequestService::Error, /already closed/) + end + + it "closes an open PR for the owner" do + pr = repo_on_disk.pull_requests.create!(user: owner, number: 1, title: "X", head_ref: "feature", base_ref: "main") + described_class.close(pull_request: pr, actor: owner) + expect(pr.reload.state).to eq("closed") + end + + it "closes an open PR for its own author even if not the repo owner" do + # PullRequestService.create enforces author == owner, but the model + # itself doesn't, so this covers the "or author" branch directly. + pr = repo_on_disk.pull_requests.create!(user: outsider, number: 1, title: "X", head_ref: "feature", base_ref: "main") + described_class.close(pull_request: pr, actor: outsider) + expect(pr.reload.state).to eq("closed") + end + + it "refuses to close for an unrelated user" do + pr = repo_on_disk.pull_requests.create!(user: owner, number: 1, title: "X", head_ref: "feature", base_ref: "main") + stranger = User.create!(smbcloud_id: 7003, email: "stranger@example.com", username: "stranger") + + expect do + described_class.close(pull_request: pr, actor: stranger) + end.to raise_error(PullRequestService::NotAuthorized) + end + + it "won't close an already-merged PR" do + pr = repo_on_disk.pull_requests.create!(user: owner, number: 1, title: "X", head_ref: "feature", base_ref: "main", state: "merged") + expect do + described_class.close(pull_request: pr, actor: owner) + end.to raise_error(PullRequestService::Error, /can't be closed/) + end + + it "reopens a closed PR" do + pr = repo_on_disk.pull_requests.create!(user: owner, number: 1, title: "X", head_ref: "feature", base_ref: "main", state: "closed") + described_class.reopen(pull_request: pr, actor: owner) + expect(pr.reload.state).to eq("open") + end + + it "won't reopen a merged PR" do + pr = repo_on_disk.pull_requests.create!(user: owner, number: 1, title: "X", head_ref: "feature", base_ref: "main", state: "merged") + expect do + described_class.reopen(pull_request: pr, actor: owner) + end.to raise_error(PullRequestService::Error, /Only closed/) + end + + def build_repo(bare_path) + FileUtils.mkdir_p(bare_path) + system("git", "init", "--bare", bare_path, exception: true) + Dir.mktmpdir do |work| + system("git", "-C", work, "init", "-b", "main", exception: true) + system("git", "-C", work, "config", "user.email", "t@example.com", exception: true) + system("git", "-C", work, "config", "user.name", "Test", exception: true) + File.write(File.join(work, "README.md"), "# Demo\n") + system("git", "-C", work, "add", ".", exception: true) + system("git", "-C", work, "commit", "-m", "seed", exception: true) + system("git", "-C", work, "remote", "add", "origin", bare_path, exception: true) + system("git", "-C", work, "push", "origin", "main", exception: true) + + system("git", "-C", work, "checkout", "-b", "feature", exception: true) + File.write(File.join(work, "feature.txt"), "hi\n") + system("git", "-C", work, "add", ".", exception: true) + system("git", "-C", work, "commit", "-m", "feature commit", exception: true) + system("git", "-C", work, "push", "origin", "feature", exception: true) + + system("git", "-C", work, "checkout", "main", exception: true) + system("git", "-C", work, "checkout", "-b", "conflict-a", exception: true) + File.write(File.join(work, "shared.txt"), "a\n") + system("git", "-C", work, "add", ".", exception: true) + system("git", "-C", work, "commit", "-m", "conflict a", exception: true) + system("git", "-C", work, "push", "origin", "conflict-a", exception: true) + + system("git", "-C", work, "checkout", "main", exception: true) + system("git", "-C", work, "checkout", "-b", "conflict-b", exception: true) + File.write(File.join(work, "shared.txt"), "b\n") + system("git", "-C", work, "add", ".", exception: true) + system("git", "-C", work, "commit", "-m", "conflict b", exception: true) + system("git", "-C", work, "push", "origin", "conflict-b", exception: true) + end + end + end end