From e653bcdd2d636fcfc10168c7743abae7dd6c7585 Mon Sep 17 00:00:00 2001 From: zogstrip Date: Fri, 28 Nov 2025 12:19:44 +0100 Subject: [PATCH 1/4] FEATURE: GitHub PR live status icon I wanted a better way to show a live/up-to-date status of a PR than what the https://github.com/discourse/github-status-theme provided. First, I wanted it to work on any PR, including ones in private repositories. Second, I wanted to include the approved status, which requires a 2nd call to GitHub's API. Third, I wanted something that was less "intrusive" than the "shield-like" status, so I opted out to use the icons & color that GitHub uses (but maybe that's a bad idea). So here's what I came up with. It's basically a small initializer script that hooks into the "decorate" APIs for posts and chat messages, and sends a requests to retrieve the PR status for each GitHub pull request oneboxes. The server-side proxy is there to add the GitHub token (via the github_onebox_access_tokens site setting) to access private repositories as well as doing some post-processing to retrieve the approved status. There is no caching, neither client-side, nor server-side, as I wanted to see how it felt in production and how often we would hit any rate limits. Internal ref - t/17138 --- lib/onebox/mixins/github_auth_header.rb | 8 +- .../pull_requests_controller.rb | 31 +++ .../initializers/github-pr-status.js | 80 ++++++ .../stylesheets/common/github-pr-status.scss | 68 +++++ .../config/locales/client.en.yml | 8 + .../config/locales/server.en.yml | 6 + plugins/discourse-github/config/settings.yml | 3 + .../discourse-github/lib/github_pr_status.rb | 50 ++++ plugins/discourse-github/plugin.rb | 10 + .../spec/lib/github_pr_status_spec.rb | 224 ++++++++++++++++ .../requests/pull_requests_controller_spec.rb | 53 ++++ .../acceptance/github-pr-status-test.js | 242 ++++++++++++++++++ 12 files changed, 778 insertions(+), 5 deletions(-) create mode 100644 plugins/discourse-github/app/controllers/discourse_github/pull_requests_controller.rb create mode 100644 plugins/discourse-github/assets/javascripts/discourse/initializers/github-pr-status.js create mode 100644 plugins/discourse-github/assets/stylesheets/common/github-pr-status.scss create mode 100644 plugins/discourse-github/lib/github_pr_status.rb create mode 100644 plugins/discourse-github/spec/lib/github_pr_status_spec.rb create mode 100644 plugins/discourse-github/spec/requests/pull_requests_controller_spec.rb create mode 100644 plugins/discourse-github/test/javascripts/acceptance/github-pr-status-test.js diff --git a/lib/onebox/mixins/github_auth_header.rb b/lib/onebox/mixins/github_auth_header.rb index c96872f20e47c..5cdac6ca828ea 100644 --- a/lib/onebox/mixins/github_auth_header.rb +++ b/lib/onebox/mixins/github_auth_header.rb @@ -5,13 +5,11 @@ module Mixins module GithubAuthHeader def github_auth_header(github_org) return {} if SiteSetting.github_onebox_access_tokens.blank? - org_tokens = - SiteSetting.github_onebox_access_tokens.split("\n").map { |line| line.split("|") }.to_h - # Use the default token if no token is found for the org, - # this will be the token that used to be stored in the old - # github_onebox_access_token site setting if it was configured. + org_tokens = SiteSetting.github_onebox_access_tokens.split("\n").to_h { _1.split("|") } + token = org_tokens[github_org] || org_tokens["default"] + return {} if token.blank? { "Authorization" => "Bearer #{token}" } diff --git a/plugins/discourse-github/app/controllers/discourse_github/pull_requests_controller.rb b/plugins/discourse-github/app/controllers/discourse_github/pull_requests_controller.rb new file mode 100644 index 0000000000000..424ea3c7cc066 --- /dev/null +++ b/plugins/discourse-github/app/controllers/discourse_github/pull_requests_controller.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module DiscourseGithub + class PullRequestsController < ::ApplicationController + requires_plugin "discourse-github" + + def status + owner = params[:owner] + repo = params[:repo] + number = params[:number] + + if owner.blank? || repo.blank? || number.blank? + return( + render json: { + error: I18n.t("discourse_github.errors.missing_parameters"), + }, + status: :bad_request + ) + end + + if state = GithubPrStatus.fetch(owner, repo, number) + render json: { state: state } + else + render json: { + error: I18n.t("discourse_github.errors.failed_to_fetch_pr_status"), + }, + status: :bad_gateway + end + end + end +end diff --git a/plugins/discourse-github/assets/javascripts/discourse/initializers/github-pr-status.js b/plugins/discourse-github/assets/javascripts/discourse/initializers/github-pr-status.js new file mode 100644 index 0000000000000..93e334beff0ba --- /dev/null +++ b/plugins/discourse-github/assets/javascripts/discourse/initializers/github-pr-status.js @@ -0,0 +1,80 @@ +import { withPluginApi } from "discourse/lib/plugin-api"; +import { i18n } from "discourse-i18n"; + +const GITHUB_PR_REGEX = + /github\.com\/(?[^/]+)\/(?[^/]+)\/pull\/(?\d+)/; + +async function fetchPrStatus(owner, repo, number) { + const response = await fetch( + `/discourse-github/${owner}/${repo}/pulls/${number}/status.json` + ); + if (!response.ok) { + return null; + } + const data = await response.json(); + return data.state; +} + +function applyStatus(onebox, status) { + [...onebox.classList].forEach((cls) => { + if (cls.startsWith("--gh-status-")) { + onebox.classList.remove(cls); + } + }); + if (status) { + onebox.classList.add(`--gh-status-${status}`); + const iconContainer = onebox.querySelector(".github-icon-container"); + if (iconContainer) { + iconContainer.title = i18n(`github.pr_status.${status}`); + } + } +} + +async function processOnebox(onebox) { + if (onebox.dataset.ghPrStatus) { + return; + } + + const match = onebox.dataset.oneboxSrc?.match(GITHUB_PR_REGEX); + if (!match) { + return; + } + + const { owner, repo, number } = match.groups; + onebox.dataset.ghPrStatus = "pending"; + const status = await fetchPrStatus(owner, repo, number); + if (status) { + onebox.dataset.ghPrStatus = status; + applyStatus(onebox, status); + } +} + +function decorateElement(element) { + element.querySelectorAll(".onebox.githubpullrequest").forEach(processOnebox); +} + +export default { + name: "github-pr-status", + + initialize(container) { + const siteSettings = container.lookup("service:site-settings"); + if (!siteSettings.github_pr_status_enabled) { + return; + } + + const currentUser = container.lookup("service:current-user"); + if (!currentUser) { + return; + } + + withPluginApi((api) => { + api.decorateCookedElement(decorateElement, { + id: "github-pr-status", + }); + + api.decorateChatMessage?.(decorateElement, { + id: "github-pr-status", + }); + }); + }, +}; diff --git a/plugins/discourse-github/assets/stylesheets/common/github-pr-status.scss b/plugins/discourse-github/assets/stylesheets/common/github-pr-status.scss new file mode 100644 index 0000000000000..f641f88fac3a7 --- /dev/null +++ b/plugins/discourse-github/assets/stylesheets/common/github-pr-status.scss @@ -0,0 +1,68 @@ +.onebox.githubpullrequest { + // Draft PR icon (dashed outline) + --gh-icon-draft: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16'%3E%3Cpath d='M3.25 1A2.25 2.25 0 0 1 4 5.372v5.256a2.251 2.251 0 1 1-1.5 0V5.372A2.251 2.251 0 0 1 3.25 1Zm9.5 14a2.25 2.25 0 1 1 0-4.5 2.25 2.25 0 0 1 0 4.5ZM2.5 3.25a.75.75 0 1 0 1.5 0 .75.75 0 0 0-1.5 0ZM3.25 12a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Zm9.5 0a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5ZM14 4.25a.75.75 0 0 1-.75.75h-2a.75.75 0 0 1 0-1.5h2a.75.75 0 0 1 .75.75Zm-.75 3.75a.75.75 0 0 0 0-1.5h-2a.75.75 0 0 0 0 1.5Z'/%3E%3C/svg%3E"); + + // Open PR icon + --gh-icon-open: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16'%3E%3Cpath 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.5Zm8.25.75a.75.75 0 1 0 1.5 0 .75.75 0 0 0-1.5 0Z'/%3E%3C/svg%3E"); + + // Merged PR icon + --gh-icon-merged: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16'%3E%3Cpath d='M5.45 5.154A4.25 4.25 0 0 0 9.25 7.5h1.378a2.251 2.251 0 1 1 0 1.5H9.25A5.734 5.734 0 0 1 5 7.123v3.505a2.25 2.25 0 1 1-1.5 0V5.372a2.25 2.25 0 1 1 1.95-.218ZM4.25 13.5a.75.75 0 1 0 0-1.5.75.75 0 0 0 0 1.5Zm8.5-4.5a.75.75 0 1 0 0-1.5.75.75 0 0 0 0 1.5ZM5 3.25a.75.75 0 1 0 0 .005V3.25Z'/%3E%3C/svg%3E"); + + // Closed PR icon + --gh-icon-closed: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16'%3E%3Cpath d='M3.25 1A2.25 2.25 0 0 1 4 5.372v5.256a2.251 2.251 0 1 1-1.5 0V5.372A2.251 2.251 0 0 1 3.25 1Zm9.5 5.5a.75.75 0 0 1 .75.75v3.378a2.251 2.251 0 1 1-1.5 0V7.25a.75.75 0 0 1 .75-.75Zm-2.03-5.273a.75.75 0 0 1 1.06 0l.97.97.97-.97a.748.748 0 0 1 1.265.332.75.75 0 0 1-.205.729l-.97.97.97.97a.751.751 0 0 1-.018 1.042.751.751 0 0 1-1.042.018l-.97-.97-.97.97a.749.749 0 0 1-1.275-.326.749.749 0 0 1 .215-.734l.97-.97-.97-.97a.75.75 0 0 1 0-1.06ZM2.5 3.25a.75.75 0 1 0 1.5 0 .75.75 0 0 0-1.5 0ZM3.25 12a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Zm9.5 0a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Z'/%3E%3C/svg%3E"); + + // Approved check-circle icon + --gh-icon-approved: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16'%3E%3Cpath d='M8 16A8 8 0 1 1 8 0a8 8 0 0 1 0 16Zm3.78-9.72a.751.751 0 0 0-.018-1.042.751.751 0 0 0-1.042-.018L6.75 9.19 5.28 7.72a.751.751 0 0 0-1.042.018.751.751 0 0 0-.018 1.042l2 2a.75.75 0 0 0 1.06 0Z'/%3E%3C/svg%3E"); + + &.--gh-status-draft, + &.--gh-status-open, + &.--gh-status-approved, + &.--gh-status-merged, + &.--gh-status-closed { + .github-icon-container { + display: block; + grid-area: icon; + align-self: center; + justify-self: center; + + .github-icon { + display: none; + } + + &::before { + content: ""; + display: block; + width: 2.5em; + aspect-ratio: 12 / 16; + mask-size: 100% 100%; + mask-repeat: no-repeat; + mask-position: center; + } + } + } + + &.--gh-status-draft .github-icon-container::before { + mask-image: var(--gh-icon-draft); + background-color: #8b949e; + } + + &.--gh-status-open .github-icon-container::before { + mask-image: var(--gh-icon-open); + background-color: #3fb950; + } + + &.--gh-status-approved .github-icon-container::before { + mask-image: var(--gh-icon-approved); + background-color: #3fb950; + } + + &.--gh-status-merged .github-icon-container::before { + mask-image: var(--gh-icon-merged); + background-color: #a371f7; + } + + &.--gh-status-closed .github-icon-container::before { + mask-image: var(--gh-icon-closed); + background-color: #f85149; + } +} diff --git a/plugins/discourse-github/config/locales/client.en.yml b/plugins/discourse-github/config/locales/client.en.yml index c3815f45d865d..f8b732d4bc8d1 100644 --- a/plugins/discourse-github/config/locales/client.en.yml +++ b/plugins/discourse-github/config/locales/client.en.yml @@ -1,4 +1,12 @@ en: + js: + github: + pr_status: + draft: "Draft" + open: "Open" + approved: "Approved" + merged: "Merged" + closed: "Closed" admin_js: admin: site_settings: diff --git a/plugins/discourse-github/config/locales/server.en.yml b/plugins/discourse-github/config/locales/server.en.yml index cfe12d2607d20..258133b98cd65 100644 --- a/plugins/discourse-github/config/locales/server.en.yml +++ b/plugins/discourse-github/config/locales/server.en.yml @@ -6,6 +6,7 @@ en: github_linkback_access_token: 'A valid access token for the user who will post the linkback and for counting commits/contributions to grant badges. See here for instructions on how to get a token.' github_linkback_maximum_links: "Maximum number of linkbacks to create from one single post. When a post contains more links, none is created." github_permalinks_enabled: "Enable GitHub permalink overwrites" + github_pr_status_enabled: "Show GitHub pull request status on oneboxes (draft, open, approved, changes requested, merged, closed)" github_permalinks_exclude: "Filename or directory that should be excluded from permalink overwrites. Provide only filename or full path: user/repository/optional-directory/filename" github_badges_enabled: "Enable GitHub badges" github_badges_repos: "URLs of the GitHub repos to scan for contributions and commits" @@ -16,6 +17,11 @@ en: invalid_badge_repo: "You must provide a GitHub URL or the repository name in the format github_user/repository_name" invalid_github_linkback_access_token: "You must provide a valid GitHub linkback access token which has access to the badge repositories you have provided." + discourse_github: + errors: + missing_parameters: "Missing parameters" + failed_to_fetch_pr_status: "Failed to fetch PR status" + github_linkback: commit_template: | This commit has been mentioned on **%{title}**. There might be relevant details there: diff --git a/plugins/discourse-github/config/settings.yml b/plugins/discourse-github/config/settings.yml index d3dbee64c4011..950d4c09fa272 100644 --- a/plugins/discourse-github/config/settings.yml +++ b/plugins/discourse-github/config/settings.yml @@ -21,6 +21,9 @@ discourse_github: validator: "GithubBadgesRepoSettingValidator" github_permalinks_enabled: default: false + github_pr_status_enabled: + default: false + client: true github_permalinks_exclude: default: "README.md" type: list diff --git a/plugins/discourse-github/lib/github_pr_status.rb b/plugins/discourse-github/lib/github_pr_status.rb new file mode 100644 index 0000000000000..a260bc030c658 --- /dev/null +++ b/plugins/discourse-github/lib/github_pr_status.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +class GithubPrStatus + extend Onebox::Mixins::GithubAuthHeader + + def self.fetch(owner, repo, number) + pr_data = fetch_json("https://api.github.com/repos/#{owner}/#{repo}/pulls/#{number}", owner) + + return "merged" if pr_data["merged"] + return "closed" if pr_data["state"] == "closed" + return "draft" if pr_data["draft"] + + reviews_data = + fetch_json("https://api.github.com/repos/#{owner}/#{repo}/pulls/#{number}/reviews", owner) + return "approved" if approved?(reviews_data) + + "open" + rescue StandardError => e + Rails.logger.error("GitHub PR status error: #{e.message}") + nil + end + + private + + def self.fetch_json(url, owner) + uri = URI.parse(url) + response = uri.open({ read_timeout: 10 }.merge(github_auth_header(owner))) + ::MultiJson.load(response.read) + end + + def self.approved?(reviews) + return false if reviews.blank? + + latest_by_user = {} + reviews.each do |review| + next unless user_id = review.dig("user", "id") + next if review["state"] == "PENDING" || review["state"] == "COMMENTED" + + existing = latest_by_user[user_id] + if existing.nil? || review["submitted_at"] > existing["submitted_at"] + latest_by_user[user_id] = review + end + end + + return false if latest_by_user.empty? + + states = latest_by_user.values.map { |r| r["state"] } + states.all? { |s| s == "APPROVED" || s == "DISMISSED" } && states.include?("APPROVED") + end +end diff --git a/plugins/discourse-github/plugin.rb b/plugins/discourse-github/plugin.rb index 2b9b94d14fa30..988724f365494 100644 --- a/plugins/discourse-github/plugin.rb +++ b/plugins/discourse-github/plugin.rb @@ -16,7 +16,17 @@ enabled_site_setting :enable_discourse_github_plugin +register_asset "stylesheets/common/github-pr-status.scss" + after_initialize do + require_relative "lib/github_pr_status" + require_relative "app/controllers/discourse_github/pull_requests_controller" + + Discourse::Application.routes.append do + get "/discourse-github/:owner/:repo/pulls/:number/status" => + "discourse_github/pull_requests#status" + end + %w[ ../app/models/github_commit.rb ../app/models/github_repo.rb diff --git a/plugins/discourse-github/spec/lib/github_pr_status_spec.rb b/plugins/discourse-github/spec/lib/github_pr_status_spec.rb new file mode 100644 index 0000000000000..79ee70262add9 --- /dev/null +++ b/plugins/discourse-github/spec/lib/github_pr_status_spec.rb @@ -0,0 +1,224 @@ +# frozen_string_literal: true + +describe GithubPrStatus do + let(:owner) { "discourse" } + let(:repo) { "discourse" } + let(:pr_number) { 123 } + + let(:pr_url) { "https://api.github.com/repos/#{owner}/#{repo}/pulls/#{pr_number}" } + let(:reviews_url) { "https://api.github.com/repos/#{owner}/#{repo}/pulls/#{pr_number}/reviews" } + + before { enable_current_plugin } + + def stub_pr_response(body) + stub_request(:get, pr_url).to_return(status: 200, body: body.to_json, headers: {}) + end + + def stub_reviews_response(body) + stub_request(:get, reviews_url).to_return(status: 200, body: body.to_json, headers: {}) + end + + describe ".fetch" do + it "returns 'merged' when the PR is merged" do + stub_pr_response({ "merged" => true, "state" => "closed" }) + + expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("merged") + end + + it "returns 'closed' when the PR is closed but not merged" do + stub_pr_response({ "merged" => false, "state" => "closed" }) + + expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("closed") + end + + it "returns 'draft' when the PR is a draft" do + stub_pr_response({ "merged" => false, "state" => "open", "draft" => true }) + + expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("draft") + end + + it "returns 'approved' when the PR has only approved reviews" do + stub_pr_response({ "merged" => false, "state" => "open", "draft" => false }) + stub_reviews_response( + [ + { + "user" => { + "id" => 1, + }, + "state" => "APPROVED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + ], + ) + + expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("approved") + end + + it "returns 'open' when the PR has no reviews" do + stub_pr_response({ "merged" => false, "state" => "open", "draft" => false }) + stub_reviews_response([]) + + expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("open") + end + + it "returns 'open' when the PR has changes requested" do + stub_pr_response({ "merged" => false, "state" => "open", "draft" => false }) + stub_reviews_response( + [ + { + "user" => { + "id" => 1, + }, + "state" => "CHANGES_REQUESTED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + ], + ) + + expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("open") + end + + it "returns nil when the API request fails" do + stub_request(:get, pr_url).to_return(status: 404, body: "", headers: {}) + + expect(GithubPrStatus.fetch(owner, repo, pr_number)).to be_nil + end + end + + describe ".approved?" do + it "returns false when reviews is empty" do + expect(GithubPrStatus.send(:approved?, [])).to eq(false) + end + + it "returns false when reviews is nil" do + expect(GithubPrStatus.send(:approved?, nil)).to eq(false) + end + + it "returns true when all reviews are approved" do + reviews = [ + { + "user" => { + "id" => 1, + }, + "state" => "APPROVED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + { + "user" => { + "id" => 2, + }, + "state" => "APPROVED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + ] + expect(GithubPrStatus.send(:approved?, reviews)).to eq(true) + end + + it "returns true when reviews are approved or dismissed with at least one approval" do + reviews = [ + { + "user" => { + "id" => 1, + }, + "state" => "APPROVED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + { + "user" => { + "id" => 2, + }, + "state" => "DISMISSED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + ] + expect(GithubPrStatus.send(:approved?, reviews)).to eq(true) + end + + it "returns false when all reviews are dismissed" do + reviews = [ + { + "user" => { + "id" => 1, + }, + "state" => "DISMISSED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + ] + expect(GithubPrStatus.send(:approved?, reviews)).to eq(false) + end + + it "returns false when any review requests changes" do + reviews = [ + { + "user" => { + "id" => 1, + }, + "state" => "APPROVED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + { + "user" => { + "id" => 2, + }, + "state" => "CHANGES_REQUESTED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + ] + expect(GithubPrStatus.send(:approved?, reviews)).to eq(false) + end + + it "uses the latest review from each user" do + reviews = [ + { + "user" => { + "id" => 1, + }, + "state" => "CHANGES_REQUESTED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + { + "user" => { + "id" => 1, + }, + "state" => "APPROVED", + "submitted_at" => "2024-01-02T00:00:00Z", + }, + ] + expect(GithubPrStatus.send(:approved?, reviews)).to eq(true) + end + + it "ignores PENDING reviews" do + reviews = [ + { + "user" => { + "id" => 1, + }, + "state" => "APPROVED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + { "user" => { "id" => 2 }, "state" => "PENDING", "submitted_at" => "2024-01-02T00:00:00Z" }, + ] + expect(GithubPrStatus.send(:approved?, reviews)).to eq(true) + end + + it "ignores COMMENTED reviews" do + reviews = [ + { + "user" => { + "id" => 1, + }, + "state" => "APPROVED", + "submitted_at" => "2024-01-01T00:00:00Z", + }, + { + "user" => { + "id" => 2, + }, + "state" => "COMMENTED", + "submitted_at" => "2024-01-02T00:00:00Z", + }, + ] + expect(GithubPrStatus.send(:approved?, reviews)).to eq(true) + end + end +end diff --git a/plugins/discourse-github/spec/requests/pull_requests_controller_spec.rb b/plugins/discourse-github/spec/requests/pull_requests_controller_spec.rb new file mode 100644 index 0000000000000..9321814d6d89b --- /dev/null +++ b/plugins/discourse-github/spec/requests/pull_requests_controller_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +describe DiscourseGithub::PullRequestsController do + let(:owner) { "discourse" } + let(:repo) { "discourse" } + let(:pr_number) { 123 } + + before { enable_current_plugin } + + describe "#status" do + it "returns the PR status when successful" do + GithubPrStatus.stubs(:fetch).with(owner, repo, pr_number.to_s).returns("open") + + get "/discourse-github/#{owner}/#{repo}/pulls/#{pr_number}/status.json" + + expect(response.status).to eq(200) + expect(response.parsed_body["state"]).to eq("open") + end + + it "returns bad_gateway when GithubPrStatus returns nil" do + GithubPrStatus.stubs(:fetch).with(owner, repo, pr_number.to_s).returns(nil) + + get "/discourse-github/#{owner}/#{repo}/pulls/#{pr_number}/status.json" + + expect(response.status).to eq(502) + expect(response.parsed_body["error"]).to eq( + I18n.t("discourse_github.errors.failed_to_fetch_pr_status"), + ) + end + + %w[merged closed draft approved open].each do |state| + it "returns '#{state}' status correctly" do + GithubPrStatus.stubs(:fetch).with(owner, repo, pr_number.to_s).returns(state) + + get "/discourse-github/#{owner}/#{repo}/pulls/#{pr_number}/status.json" + + expect(response.status).to eq(200) + expect(response.parsed_body["state"]).to eq(state) + end + end + + it "handles hyphens in owner and repo names" do + special_owner = "my-org" + special_repo = "my-repo" + GithubPrStatus.stubs(:fetch).with(special_owner, special_repo, pr_number.to_s).returns("open") + + get "/discourse-github/#{special_owner}/#{special_repo}/pulls/#{pr_number}/status.json" + + expect(response.status).to eq(200) + expect(response.parsed_body["state"]).to eq("open") + end + end +end diff --git a/plugins/discourse-github/test/javascripts/acceptance/github-pr-status-test.js b/plugins/discourse-github/test/javascripts/acceptance/github-pr-status-test.js new file mode 100644 index 0000000000000..a9d73e262b1b4 --- /dev/null +++ b/plugins/discourse-github/test/javascripts/acceptance/github-pr-status-test.js @@ -0,0 +1,242 @@ +import { visit } from "@ember/test-helpers"; +import { test } from "qunit"; +import { acceptance } from "discourse/tests/helpers/qunit-helpers"; +import { i18n } from "discourse-i18n"; + +const PR_ONEBOX_HTML = ` + +`; + +function topicWithGithubPrOnebox() { + return { + post_stream: { + posts: [ + { + id: 1, + username: "test_user", + avatar_template: "/letter_avatar_proxy/v2/letter/t/ac91a4/{size}.png", + created_at: "2024-01-01T00:00:00.000Z", + cooked: PR_ONEBOX_HTML, + post_number: 1, + post_type: 1, + updated_at: "2024-01-01T00:00:00.000Z", + reply_count: 0, + reply_to_post_number: null, + quote_count: 0, + incoming_link_count: 0, + reads: 1, + score: 0, + yours: false, + topic_id: 1, + topic_slug: "test-topic", + version: 1, + can_edit: false, + can_delete: false, + can_recover: false, + can_wiki: false, + read: true, + actions_summary: [], + moderator: false, + admin: false, + staff: false, + user_id: 2, + hidden: false, + trust_level: 1, + deleted_at: null, + user_deleted: false, + can_view_edit_history: true, + wiki: false, + }, + ], + stream: [1], + }, + timeline_lookup: [[1, 0]], + id: 1, + title: "Test Topic with GitHub PR", + fancy_title: "Test Topic with GitHub PR", + posts_count: 1, + created_at: "2024-01-01T00:00:00.000Z", + views: 1, + reply_count: 0, + participant_count: 1, + like_count: 0, + last_posted_at: "2024-01-01T00:00:00.000Z", + visible: true, + closed: false, + archived: false, + has_summary: false, + archetype: "regular", + slug: "test-topic", + category_id: 1, + word_count: 10, + deleted_at: null, + user_id: 2, + draft: null, + draft_key: "topic_1", + draft_sequence: 0, + posted: false, + pinned_globally: false, + pinned: false, + details: { + created_by: { + id: 2, + username: "test_user", + avatar_template: "/letter_avatar_proxy/v2/letter/t/ac91a4/{size}.png", + }, + last_poster: { + id: 2, + username: "test_user", + avatar_template: "/letter_avatar_proxy/v2/letter/t/ac91a4/{size}.png", + }, + participants: [ + { + id: 2, + username: "test_user", + avatar_template: "/letter_avatar_proxy/v2/letter/t/ac91a4/{size}.png", + post_count: 1, + }, + ], + notification_level: 1, + can_create_post: true, + can_reply_as_new_topic: true, + can_flag_topic: true, + }, + highest_post_number: 1, + last_read_post_number: 1, + last_read_post_id: 1, + has_deleted: false, + actions_summary: [], + chunk_size: 20, + bookmarked: false, + tags: [], + message_bus_last_id: 0, + }; +} + +acceptance("Discourse GitHub | PR Status", function (needs) { + needs.user(); + + needs.settings({ + enable_discourse_github_plugin: true, + github_pr_status_enabled: true, + }); + + needs.pretender((server, helper) => { + server.get("/t/1.json", () => helper.response(topicWithGithubPrOnebox())); + + server.get("/discourse-github/:owner/:repo/pulls/:number/status.json", () => + helper.response({ state: "merged" }) + ); + }); + + test("applies PR status class to onebox", async function (assert) { + await visit("/t/test-topic/1"); + + assert + .dom(".onebox.githubpullrequest") + .hasClass("--gh-status-merged", "onebox has merged status class"); + + assert + .dom(".onebox.githubpullrequest") + .hasAttribute( + "data-gh-pr-status", + "merged", + "onebox has data attribute with status" + ); + }); + + test("sets title on icon container", async function (assert) { + await visit("/t/test-topic/1"); + + assert + .dom(".onebox.githubpullrequest .github-icon-container") + .hasAttribute( + "title", + i18n("github.pr_status.merged"), + "icon container has correct title" + ); + }); +}); + +acceptance("Discourse GitHub | PR Status - Different States", function (needs) { + needs.user(); + + needs.settings({ + enable_discourse_github_plugin: true, + github_pr_status_enabled: true, + }); + + needs.pretender((server, helper) => { + server.get("/t/1.json", () => helper.response(topicWithGithubPrOnebox())); + + server.get("/discourse-github/:owner/:repo/pulls/:number/status.json", () => + helper.response({ state: "open" }) + ); + }); + + test("applies open status class", async function (assert) { + await visit("/t/test-topic/1"); + + assert + .dom(".onebox.githubpullrequest") + .hasClass("--gh-status-open", "onebox has open status class"); + }); +}); + +acceptance("Discourse GitHub | PR Status - Disabled", function (needs) { + needs.user(); + + needs.settings({ + enable_discourse_github_plugin: true, + github_pr_status_enabled: false, + }); + + needs.pretender((server, helper) => { + server.get("/t/1.json", () => helper.response(topicWithGithubPrOnebox())); + }); + + test("does not fetch status when disabled", async function (assert) { + await visit("/t/test-topic/1"); + + assert + .dom(".onebox.githubpullrequest") + .doesNotHaveClass( + "--gh-status-merged", + "onebox does not have status class when disabled" + ); + + assert + .dom(".onebox.githubpullrequest[data-gh-pr-status]") + .doesNotExist("onebox does not have data attribute when disabled"); + }); +}); + +acceptance("Discourse GitHub | PR Status - Anonymous User", function (needs) { + needs.settings({ + enable_discourse_github_plugin: true, + github_pr_status_enabled: true, + }); + + needs.pretender((server, helper) => { + server.get("/t/1.json", () => helper.response(topicWithGithubPrOnebox())); + }); + + test("does not fetch status for anonymous users", async function (assert) { + await visit("/t/test-topic/1"); + + assert + .dom(".onebox.githubpullrequest[data-gh-pr-status]") + .doesNotExist("onebox does not have data attribute for anonymous users"); + }); +}); From 0e2d7ab7da640b1959d9a947ec492406cb47e1e7 Mon Sep 17 00:00:00 2001 From: zogstrip Date: Sat, 29 Nov 2025 10:46:21 +0100 Subject: [PATCH 2/4] add GH rate limit-handling and logging --- .../discourse-github/lib/github_pr_status.rb | 7 +++++++ .../spec/lib/github_pr_status_spec.rb | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/plugins/discourse-github/lib/github_pr_status.rb b/plugins/discourse-github/lib/github_pr_status.rb index a260bc030c658..9ec612c235a3a 100644 --- a/plugins/discourse-github/lib/github_pr_status.rb +++ b/plugins/discourse-github/lib/github_pr_status.rb @@ -26,6 +26,13 @@ def self.fetch_json(url, owner) uri = URI.parse(url) response = uri.open({ read_timeout: 10 }.merge(github_auth_header(owner))) ::MultiJson.load(response.read) + rescue OpenURI::HTTPError => e + if e.io.status[0] == "403" && e.io.meta["x-ratelimit-remaining"] == "0" + reset_time = e.io.meta["x-ratelimit-reset"]&.to_i + reset_at = reset_time ? Time.at(reset_time).utc : "unknown" + Rails.logger.warn("GitHub API rate limit exceeded for #{url}. Resets at: #{reset_at}.") + end + raise end def self.approved?(reviews) diff --git a/plugins/discourse-github/spec/lib/github_pr_status_spec.rb b/plugins/discourse-github/spec/lib/github_pr_status_spec.rb index 79ee70262add9..291130dac5bea 100644 --- a/plugins/discourse-github/spec/lib/github_pr_status_spec.rb +++ b/plugins/discourse-github/spec/lib/github_pr_status_spec.rb @@ -83,6 +83,25 @@ def stub_reviews_response(body) expect(GithubPrStatus.fetch(owner, repo, pr_number)).to be_nil end + + it "logs a warning when rate limited" do + reset_time = Time.now.to_i + 3600 + + stub_request(:get, pr_url).to_return( + status: 403, + body: "", + headers: { + "x-ratelimit-remaining" => "0", + "x-ratelimit-reset" => reset_time.to_s, + }, + ) + + Rails.logger.expects(:warn).with(regexp_matches(/GitHub API rate limit exceeded/)) + + Rails.logger.expects(:error).with(regexp_matches(/GitHub PR status error/)) + + expect(GithubPrStatus.fetch(owner, repo, pr_number)).to be_nil + end end describe ".approved?" do From 3b5ae2951ba3357961ac379adca247930e5a164a Mon Sep 17 00:00:00 2001 From: zogstrip Date: Wed, 3 Dec 2025 15:00:16 +0100 Subject: [PATCH 3/4] FEATURE: add support for github webhook to keep PR oneboxes up-to-date FEATURE: add "Chat::MessageLink" which keeps track of links shared in chat messages Internal ref - t/ --- .../engine/github_pull_request_onebox.rb | 32 +++ .../templates/githubpullrequest.mustache | 2 +- .../app/jobs/regular/chat/process_message.rb | 3 + plugins/chat/app/models/chat/message_link.rb | 65 +++++ ...0251203000001_create_chat_message_links.rb | 22 ++ .../spec/models/chat/message_link_spec.rb | 118 +++++++++ .../pull_requests_controller.rb | 31 --- .../discourse_github/webhooks_controller.rb | 40 +++ .../jobs/regular/rebake_github_pr_posts.rb | 54 ++++ .../initializers/github-pr-status.js | 80 ------ .../stylesheets/common/github-pr-status.scss | 2 +- .../config/locales/server.en.yml | 6 +- plugins/discourse-github/config/settings.yml | 4 +- .../discourse-github/lib/github_pr_status.rb | 57 ---- plugins/discourse-github/plugin.rb | 7 +- .../spec/jobs/rebake_github_pr_posts_spec.rb | 97 +++++++ .../spec/lib/github_pr_onebox_status_spec.rb | 111 ++++++++ .../spec/lib/github_pr_status_spec.rb | 243 ------------------ .../requests/pull_requests_controller_spec.rb | 53 ---- .../spec/requests/webhooks_controller_spec.rb | 151 +++++++++++ .../acceptance/github-pr-status-test.js | 242 ----------------- 21 files changed, 702 insertions(+), 718 deletions(-) create mode 100644 plugins/chat/app/models/chat/message_link.rb create mode 100644 plugins/chat/db/migrate/20251203000001_create_chat_message_links.rb create mode 100644 plugins/chat/spec/models/chat/message_link_spec.rb delete mode 100644 plugins/discourse-github/app/controllers/discourse_github/pull_requests_controller.rb create mode 100644 plugins/discourse-github/app/controllers/discourse_github/webhooks_controller.rb create mode 100644 plugins/discourse-github/app/jobs/regular/rebake_github_pr_posts.rb delete mode 100644 plugins/discourse-github/assets/javascripts/discourse/initializers/github-pr-status.js delete mode 100644 plugins/discourse-github/lib/github_pr_status.rb create mode 100644 plugins/discourse-github/spec/jobs/rebake_github_pr_posts_spec.rb create mode 100644 plugins/discourse-github/spec/lib/github_pr_onebox_status_spec.rb delete mode 100644 plugins/discourse-github/spec/lib/github_pr_status_spec.rb delete mode 100644 plugins/discourse-github/spec/requests/pull_requests_controller_spec.rb create mode 100644 plugins/discourse-github/spec/requests/webhooks_controller_spec.rb delete mode 100644 plugins/discourse-github/test/javascripts/acceptance/github-pr-status-test.js diff --git a/lib/onebox/engine/github_pull_request_onebox.rb b/lib/onebox/engine/github_pull_request_onebox.rb index 4588d8ea4250b..813918387d4a8 100644 --- a/lib/onebox/engine/github_pull_request_onebox.rb +++ b/lib/onebox/engine/github_pull_request_onebox.rb @@ -33,6 +33,7 @@ def match def data result = raw(github_auth_header(match[:org])).clone result["link"] = link + result["pr_status"] = fetch_pr_status(result) created_at = Time.parse(result["created_at"]) result["created_at"] = created_at.strftime("%I:%M%p - %d %b %y %Z") @@ -112,6 +113,37 @@ def load_json(url) URI.parse(url).open({ read_timeout: timeout }.merge(github_auth_header(match[:org]))), ) end + + def fetch_pr_status(pr_data) + return unless SiteSetting.github_pr_status_enabled + + return "merged" if pr_data["merged"] + return "closed" if pr_data["state"] == "closed" + return "draft" if pr_data["draft"] + + reviews_data = load_json(url + "/reviews") + return "approved" if reviews_approved?(reviews_data) + + "open" + rescue StandardError => e + Rails.logger.warn("GitHub PR status fetch error: #{e.message}") + nil + end + + def reviews_approved?(reviews) + return false if reviews.blank? + + states = + reviews + .reject { |r| r.dig("user", "id").nil? || %w[PENDING COMMENTED].include?(r["state"]) } + .group_by { |r| r.dig("user", "id") } + .transform_values { |rs| rs.max_by { |r| r["submitted_at"] }["state"] } + .values + + return false if states.empty? + + states.all? { |s| %w[APPROVED DISMISSED].include?(s) } && states.include?("APPROVED") + end end end end diff --git a/lib/onebox/templates/githubpullrequest.mustache b/lib/onebox/templates/githubpullrequest.mustache index 9bb3299bbcbd9..9383d67ed64ee 100644 --- a/lib/onebox/templates/githubpullrequest.mustache +++ b/lib/onebox/templates/githubpullrequest.mustache @@ -1,4 +1,4 @@ -
+
{{#commit}}
diff --git a/plugins/chat/app/jobs/regular/chat/process_message.rb b/plugins/chat/app/jobs/regular/chat/process_message.rb index 697c4e352d0f8..683db70535e6f 100644 --- a/plugins/chat/app/jobs/regular/chat/process_message.rb +++ b/plugins/chat/app/jobs/regular/chat/process_message.rb @@ -31,6 +31,9 @@ def execute(args = {}) # we dont process mentions when creating/updating message so we always have to do it chat_message.upsert_mentions + # extract external links for webhook-based rebaking + ::Chat::MessageLink.extract_from(chat_message) + # notifier should be idempotent and not re-notify if args[:edit_timestamp] ::Chat::Notifier.new(chat_message, args[:edit_timestamp]).notify_edit diff --git a/plugins/chat/app/models/chat/message_link.rb b/plugins/chat/app/models/chat/message_link.rb new file mode 100644 index 0000000000000..19ddd55e89d1b --- /dev/null +++ b/plugins/chat/app/models/chat/message_link.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +module Chat + class MessageLink < ActiveRecord::Base + self.table_name = "chat_message_links" + + belongs_to :chat_message, class_name: "Chat::Message" + belongs_to :chat_channel, class_name: "Chat::Channel" + belongs_to :user + + validates :url, presence: true, length: { maximum: 500 } + + def self.extract_from(message) + return if message.blank? || message.user_id.blank? || message.deleted_at.present? + + current_urls = [] + + PrettyText + .extract_links(message.cooked) + .map { |link| UrlHelper.relaxed_parse(link.url) } + .compact + .reject { |uri| uri.scheme == "mailto" } + .uniq + .each do |parsed| + url = parsed.to_s[0...500] + next if parsed.host.blank? || parsed.host.length > 100 + + current_urls << url + upsert_link(message, url, parsed.host) + end + + cleanup_entries(message, current_urls) + end + + def self.upsert_link(message, url, domain) + sql = <<~SQL + INSERT INTO chat_message_links (chat_message_id, chat_channel_id, user_id, url, domain, created_at, updated_at) + VALUES (:chat_message_id, :chat_channel_id, :user_id, :url, :domain, :now, :now) + ON CONFLICT (chat_message_id, url) DO NOTHING + SQL + + DB.exec( + sql, + chat_message_id: message.id, + chat_channel_id: message.chat_channel_id, + user_id: message.user_id, + url: url, + domain: domain, + now: Time.current, + ) + end + + def self.cleanup_entries(message, current_urls) + if current_urls.present? + where( + "chat_message_id = :id AND url NOT IN (:urls)", + id: message.id, + urls: current_urls, + ).delete_all + else + where(chat_message_id: message.id).delete_all + end + end + end +end diff --git a/plugins/chat/db/migrate/20251203000001_create_chat_message_links.rb b/plugins/chat/db/migrate/20251203000001_create_chat_message_links.rb new file mode 100644 index 0000000000000..64d2ca95a8a87 --- /dev/null +++ b/plugins/chat/db/migrate/20251203000001_create_chat_message_links.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class CreateChatMessageLinks < ActiveRecord::Migration[7.2] + def change + create_table :chat_message_links do |t| + t.bigint :chat_message_id, null: false + t.bigint :chat_channel_id, null: false + t.integer :user_id, null: false + t.string :url, null: false, limit: 500 + t.string :domain, limit: 100 + t.timestamps + end + + add_index :chat_message_links, :chat_message_id + add_index :chat_message_links, :chat_channel_id + add_index :chat_message_links, :url + add_index :chat_message_links, + %i[chat_message_id url], + unique: true, + name: "unique_chat_message_links" + end +end diff --git a/plugins/chat/spec/models/chat/message_link_spec.rb b/plugins/chat/spec/models/chat/message_link_spec.rb new file mode 100644 index 0000000000000..9d51b505dd59f --- /dev/null +++ b/plugins/chat/spec/models/chat/message_link_spec.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +RSpec.describe Chat::MessageLink do + fab!(:user) + fab!(:channel, :chat_channel) + + describe ".extract_from" do + it "extracts external links from a chat message" do + message = + Fabricate( + :chat_message, + chat_channel: channel, + user: user, + message: "Check out https://github.com/discourse/discourse/pull/123", + ) + message.update!(cooked: PrettyText.cook(message.message)) + + Chat::MessageLink.extract_from(message) + + expect(Chat::MessageLink.count).to eq(1) + link = Chat::MessageLink.first + expect(link.url).to eq("https://github.com/discourse/discourse/pull/123") + expect(link.domain).to eq("github.com") + expect(link.chat_message_id).to eq(message.id) + expect(link.chat_channel_id).to eq(channel.id) + expect(link.user_id).to eq(user.id) + end + + it "extracts multiple links from a message" do + message = + Fabricate( + :chat_message, + chat_channel: channel, + user: user, + message: "PRs: https://github.com/org/repo/pull/1 and https://github.com/org/repo/pull/2", + ) + message.update!(cooked: PrettyText.cook(message.message)) + + Chat::MessageLink.extract_from(message) + + expect(Chat::MessageLink.count).to eq(2) + urls = Chat::MessageLink.pluck(:url) + expect(urls).to contain_exactly( + "https://github.com/org/repo/pull/1", + "https://github.com/org/repo/pull/2", + ) + end + + it "removes links that no longer exist in the message" do + message = + Fabricate( + :chat_message, + chat_channel: channel, + user: user, + message: "Check out https://github.com/org/repo/pull/1", + ) + message.update!(cooked: PrettyText.cook(message.message)) + Chat::MessageLink.extract_from(message) + + expect(Chat::MessageLink.count).to eq(1) + + message.update!( + message: "Check out https://github.com/org/repo/pull/2", + cooked: PrettyText.cook("Check out https://github.com/org/repo/pull/2"), + ) + Chat::MessageLink.extract_from(message) + + expect(Chat::MessageLink.count).to eq(1) + expect(Chat::MessageLink.first.url).to eq("https://github.com/org/repo/pull/2") + end + + it "does not extract mailto links" do + message = + Fabricate( + :chat_message, + chat_channel: channel, + user: user, + message: "Email me at mailto:test@example.com", + ) + message.update!(cooked: PrettyText.cook(message.message)) + + Chat::MessageLink.extract_from(message) + + expect(Chat::MessageLink.count).to eq(0) + end + + it "does nothing for deleted messages" do + message = + Fabricate( + :chat_message, + chat_channel: channel, + user: user, + message: "https://github.com/org/repo/pull/1", + deleted_at: Time.current, + ) + message.update!(cooked: PrettyText.cook(message.message)) + + Chat::MessageLink.extract_from(message) + + expect(Chat::MessageLink.count).to eq(0) + end + + it "is idempotent" do + message = + Fabricate( + :chat_message, + chat_channel: channel, + user: user, + message: "https://github.com/org/repo/pull/1", + ) + message.update!(cooked: PrettyText.cook(message.message)) + + 3.times { Chat::MessageLink.extract_from(message) } + + expect(Chat::MessageLink.count).to eq(1) + end + end +end diff --git a/plugins/discourse-github/app/controllers/discourse_github/pull_requests_controller.rb b/plugins/discourse-github/app/controllers/discourse_github/pull_requests_controller.rb deleted file mode 100644 index 424ea3c7cc066..0000000000000 --- a/plugins/discourse-github/app/controllers/discourse_github/pull_requests_controller.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -module DiscourseGithub - class PullRequestsController < ::ApplicationController - requires_plugin "discourse-github" - - def status - owner = params[:owner] - repo = params[:repo] - number = params[:number] - - if owner.blank? || repo.blank? || number.blank? - return( - render json: { - error: I18n.t("discourse_github.errors.missing_parameters"), - }, - status: :bad_request - ) - end - - if state = GithubPrStatus.fetch(owner, repo, number) - render json: { state: state } - else - render json: { - error: I18n.t("discourse_github.errors.failed_to_fetch_pr_status"), - }, - status: :bad_gateway - end - end - end -end diff --git a/plugins/discourse-github/app/controllers/discourse_github/webhooks_controller.rb b/plugins/discourse-github/app/controllers/discourse_github/webhooks_controller.rb new file mode 100644 index 0000000000000..94988e977bb20 --- /dev/null +++ b/plugins/discourse-github/app/controllers/discourse_github/webhooks_controller.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module DiscourseGithub + class WebhooksController < ::ApplicationController + requires_plugin "discourse-github" + + skip_before_action :check_xhr + skip_before_action :verify_authenticity_token + skip_before_action :redirect_to_login_if_required + + def github + return head :forbidden unless verify_signature + + event = request.headers["X-GitHub-Event"] + return head :ok if %w[pull_request pull_request_review].exclude?(event) + + pr_url = params.dig(:pull_request, :html_url) + return head :ok if pr_url.blank? + + Jobs.enqueue(:rebake_github_pr_posts, pr_url: pr_url) + + head :ok + end + + private + + def verify_signature + secret = SiteSetting.github_webhook_secret + return false if secret.blank? + + signature = request.headers["X-Hub-Signature-256"] + return false if signature.blank? + + body = request.body.read + expected = "sha256=" + OpenSSL::HMAC.hexdigest("SHA256", secret, body) + + ActiveSupport::SecurityUtils.secure_compare(expected, signature) + end + end +end diff --git a/plugins/discourse-github/app/jobs/regular/rebake_github_pr_posts.rb b/plugins/discourse-github/app/jobs/regular/rebake_github_pr_posts.rb new file mode 100644 index 0000000000000..6214f14ec43f4 --- /dev/null +++ b/plugins/discourse-github/app/jobs/regular/rebake_github_pr_posts.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module Jobs + class RebakeGithubPrPosts < ::Jobs::Base + sidekiq_options queue: "low" + + def execute(args) + pr_url = args[:pr_url] + return if pr_url.blank? + + rebake_posts(pr_url) + rebake_chat_messages(pr_url) if defined?(Chat) + end + + private + + def rebake_posts(pr_url) + post_ids = + TopicLink + .where(url: pr_url) + .or(TopicLink.where("url LIKE ?", "#{pr_url}%")) + .select(:post_id) + + Post + .where(id: post_ids) + .find_each do |post| + next unless has_github_pr_onebox?(post.cooked, pr_url) + post.rebake!(invalidate_oneboxes: true, priority: :low) + end + end + + def rebake_chat_messages(pr_url) + return unless defined?(Chat::MessageLink) + + message_ids = + Chat::MessageLink + .where(url: pr_url) + .or(Chat::MessageLink.where("url LIKE ?", "#{pr_url}%")) + .select(:chat_message_id) + + Chat::Message + .where(id: message_ids) + .find_each do |message| + next unless has_github_pr_onebox?(message.cooked, pr_url) + message.rebake!(invalidate_oneboxes: true, priority: :low) + end + end + + def has_github_pr_onebox?(cooked, pr_url) + # quick & dirty check to avoid doing unnecessary rebakes + cooked.present? && cooked.include?("onebox") && cooked.include?(pr_url) + end + end +end diff --git a/plugins/discourse-github/assets/javascripts/discourse/initializers/github-pr-status.js b/plugins/discourse-github/assets/javascripts/discourse/initializers/github-pr-status.js deleted file mode 100644 index 93e334beff0ba..0000000000000 --- a/plugins/discourse-github/assets/javascripts/discourse/initializers/github-pr-status.js +++ /dev/null @@ -1,80 +0,0 @@ -import { withPluginApi } from "discourse/lib/plugin-api"; -import { i18n } from "discourse-i18n"; - -const GITHUB_PR_REGEX = - /github\.com\/(?[^/]+)\/(?[^/]+)\/pull\/(?\d+)/; - -async function fetchPrStatus(owner, repo, number) { - const response = await fetch( - `/discourse-github/${owner}/${repo}/pulls/${number}/status.json` - ); - if (!response.ok) { - return null; - } - const data = await response.json(); - return data.state; -} - -function applyStatus(onebox, status) { - [...onebox.classList].forEach((cls) => { - if (cls.startsWith("--gh-status-")) { - onebox.classList.remove(cls); - } - }); - if (status) { - onebox.classList.add(`--gh-status-${status}`); - const iconContainer = onebox.querySelector(".github-icon-container"); - if (iconContainer) { - iconContainer.title = i18n(`github.pr_status.${status}`); - } - } -} - -async function processOnebox(onebox) { - if (onebox.dataset.ghPrStatus) { - return; - } - - const match = onebox.dataset.oneboxSrc?.match(GITHUB_PR_REGEX); - if (!match) { - return; - } - - const { owner, repo, number } = match.groups; - onebox.dataset.ghPrStatus = "pending"; - const status = await fetchPrStatus(owner, repo, number); - if (status) { - onebox.dataset.ghPrStatus = status; - applyStatus(onebox, status); - } -} - -function decorateElement(element) { - element.querySelectorAll(".onebox.githubpullrequest").forEach(processOnebox); -} - -export default { - name: "github-pr-status", - - initialize(container) { - const siteSettings = container.lookup("service:site-settings"); - if (!siteSettings.github_pr_status_enabled) { - return; - } - - const currentUser = container.lookup("service:current-user"); - if (!currentUser) { - return; - } - - withPluginApi((api) => { - api.decorateCookedElement(decorateElement, { - id: "github-pr-status", - }); - - api.decorateChatMessage?.(decorateElement, { - id: "github-pr-status", - }); - }); - }, -}; diff --git a/plugins/discourse-github/assets/stylesheets/common/github-pr-status.scss b/plugins/discourse-github/assets/stylesheets/common/github-pr-status.scss index f641f88fac3a7..0f1e9fc43623b 100644 --- a/plugins/discourse-github/assets/stylesheets/common/github-pr-status.scss +++ b/plugins/discourse-github/assets/stylesheets/common/github-pr-status.scss @@ -1,4 +1,4 @@ -.onebox.githubpullrequest { +.onebox.githubpullrequest .github-row { // Draft PR icon (dashed outline) --gh-icon-draft: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16'%3E%3Cpath d='M3.25 1A2.25 2.25 0 0 1 4 5.372v5.256a2.251 2.251 0 1 1-1.5 0V5.372A2.251 2.251 0 0 1 3.25 1Zm9.5 14a2.25 2.25 0 1 1 0-4.5 2.25 2.25 0 0 1 0 4.5ZM2.5 3.25a.75.75 0 1 0 1.5 0 .75.75 0 0 0-1.5 0ZM3.25 12a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Zm9.5 0a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5ZM14 4.25a.75.75 0 0 1-.75.75h-2a.75.75 0 0 1 0-1.5h2a.75.75 0 0 1 .75.75Zm-.75 3.75a.75.75 0 0 0 0-1.5h-2a.75.75 0 0 0 0 1.5Z'/%3E%3C/svg%3E"); diff --git a/plugins/discourse-github/config/locales/server.en.yml b/plugins/discourse-github/config/locales/server.en.yml index 258133b98cd65..f6ce1856869ed 100644 --- a/plugins/discourse-github/config/locales/server.en.yml +++ b/plugins/discourse-github/config/locales/server.en.yml @@ -7,6 +7,7 @@ en: github_linkback_maximum_links: "Maximum number of linkbacks to create from one single post. When a post contains more links, none is created." github_permalinks_enabled: "Enable GitHub permalink overwrites" github_pr_status_enabled: "Show GitHub pull request status on oneboxes (draft, open, approved, changes requested, merged, closed)" + github_webhook_secret: "Secret token for GitHub webhook signature verification. Configure this value in your GitHub webhook settings." github_permalinks_exclude: "Filename or directory that should be excluded from permalink overwrites. Provide only filename or full path: user/repository/optional-directory/filename" github_badges_enabled: "Enable GitHub badges" github_badges_repos: "URLs of the GitHub repos to scan for contributions and commits" @@ -17,11 +18,6 @@ en: invalid_badge_repo: "You must provide a GitHub URL or the repository name in the format github_user/repository_name" invalid_github_linkback_access_token: "You must provide a valid GitHub linkback access token which has access to the badge repositories you have provided." - discourse_github: - errors: - missing_parameters: "Missing parameters" - failed_to_fetch_pr_status: "Failed to fetch PR status" - github_linkback: commit_template: | This commit has been mentioned on **%{title}**. There might be relevant details there: diff --git a/plugins/discourse-github/config/settings.yml b/plugins/discourse-github/config/settings.yml index 950d4c09fa272..c9d9993bf61e8 100644 --- a/plugins/discourse-github/config/settings.yml +++ b/plugins/discourse-github/config/settings.yml @@ -23,7 +23,9 @@ discourse_github: default: false github_pr_status_enabled: default: false - client: true + github_webhook_secret: + default: "" + secret: true github_permalinks_exclude: default: "README.md" type: list diff --git a/plugins/discourse-github/lib/github_pr_status.rb b/plugins/discourse-github/lib/github_pr_status.rb deleted file mode 100644 index 9ec612c235a3a..0000000000000 --- a/plugins/discourse-github/lib/github_pr_status.rb +++ /dev/null @@ -1,57 +0,0 @@ -# frozen_string_literal: true - -class GithubPrStatus - extend Onebox::Mixins::GithubAuthHeader - - def self.fetch(owner, repo, number) - pr_data = fetch_json("https://api.github.com/repos/#{owner}/#{repo}/pulls/#{number}", owner) - - return "merged" if pr_data["merged"] - return "closed" if pr_data["state"] == "closed" - return "draft" if pr_data["draft"] - - reviews_data = - fetch_json("https://api.github.com/repos/#{owner}/#{repo}/pulls/#{number}/reviews", owner) - return "approved" if approved?(reviews_data) - - "open" - rescue StandardError => e - Rails.logger.error("GitHub PR status error: #{e.message}") - nil - end - - private - - def self.fetch_json(url, owner) - uri = URI.parse(url) - response = uri.open({ read_timeout: 10 }.merge(github_auth_header(owner))) - ::MultiJson.load(response.read) - rescue OpenURI::HTTPError => e - if e.io.status[0] == "403" && e.io.meta["x-ratelimit-remaining"] == "0" - reset_time = e.io.meta["x-ratelimit-reset"]&.to_i - reset_at = reset_time ? Time.at(reset_time).utc : "unknown" - Rails.logger.warn("GitHub API rate limit exceeded for #{url}. Resets at: #{reset_at}.") - end - raise - end - - def self.approved?(reviews) - return false if reviews.blank? - - latest_by_user = {} - reviews.each do |review| - next unless user_id = review.dig("user", "id") - next if review["state"] == "PENDING" || review["state"] == "COMMENTED" - - existing = latest_by_user[user_id] - if existing.nil? || review["submitted_at"] > existing["submitted_at"] - latest_by_user[user_id] = review - end - end - - return false if latest_by_user.empty? - - states = latest_by_user.values.map { |r| r["state"] } - states.all? { |s| s == "APPROVED" || s == "DISMISSED" } && states.include?("APPROVED") - end -end diff --git a/plugins/discourse-github/plugin.rb b/plugins/discourse-github/plugin.rb index 988724f365494..541e499240e93 100644 --- a/plugins/discourse-github/plugin.rb +++ b/plugins/discourse-github/plugin.rb @@ -19,12 +19,11 @@ register_asset "stylesheets/common/github-pr-status.scss" after_initialize do - require_relative "lib/github_pr_status" - require_relative "app/controllers/discourse_github/pull_requests_controller" + require_relative "app/controllers/discourse_github/webhooks_controller" + require_relative "app/jobs/regular/rebake_github_pr_posts" Discourse::Application.routes.append do - get "/discourse-github/:owner/:repo/pulls/:number/status" => - "discourse_github/pull_requests#status" + post "/discourse-github/webhooks/github" => "discourse_github/webhooks#github" end %w[ diff --git a/plugins/discourse-github/spec/jobs/rebake_github_pr_posts_spec.rb b/plugins/discourse-github/spec/jobs/rebake_github_pr_posts_spec.rb new file mode 100644 index 0000000000000..fd0fa444f9d88 --- /dev/null +++ b/plugins/discourse-github/spec/jobs/rebake_github_pr_posts_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +RSpec.describe Jobs::RebakeGithubPrPosts do + fab!(:user) + fab!(:topic) + let(:pr_url) { "https://github.com/discourse/discourse/pull/123" } + + before { enable_current_plugin } + + describe "#execute" do + it "does nothing with blank pr_url" do + expect { described_class.new.execute(pr_url: nil) }.not_to raise_error + expect { described_class.new.execute(pr_url: "") }.not_to raise_error + end + + context "with oneboxed PR link" do + fab!(:post) { Fabricate(:post, topic: topic, user: user) } + + before do + post.update!(cooked: <<~HTML) +

Check out this PR:

+ + HTML + + TopicLink.create!( + topic_id: topic.id, + post_id: post.id, + user_id: user.id, + url: pr_url, + domain: "github.com", + ) + end + + it "rebakes posts with oneboxed PR links" do + expect_any_instance_of(Post).to receive(:rebake!).with( + invalidate_oneboxes: true, + priority: :low, + ) + described_class.new.execute(pr_url: pr_url) + end + + it "matches URLs with path suffixes" do + TopicLink.create!( + topic_id: topic.id, + post_id: post.id, + user_id: user.id, + url: "#{pr_url}/files", + domain: "github.com", + ) + + post_ids = + TopicLink + .where(url: pr_url) + .or(TopicLink.where("url LIKE ?", "#{pr_url}%")) + .pluck(:post_id) + + expect(post_ids).to include(post.id) + end + end + + context "with inline PR link (not oneboxed)" do + fab!(:post) { Fabricate(:post, topic: topic, user: user) } + + before do + post.update!(cooked: <<~HTML) +

Check out this PR for details.

+ HTML + + TopicLink.create!( + topic_id: topic.id, + post_id: post.id, + user_id: user.id, + url: pr_url, + domain: "github.com", + ) + end + + it "does not rebake posts with only inline links" do + expect_any_instance_of(Post).not_to receive(:rebake!) + described_class.new.execute(pr_url: pr_url) + end + end + + context "with no matching posts" do + it "completes without error" do + expect { described_class.new.execute(pr_url: pr_url) }.not_to raise_error + end + end + end +end diff --git a/plugins/discourse-github/spec/lib/github_pr_onebox_status_spec.rb b/plugins/discourse-github/spec/lib/github_pr_onebox_status_spec.rb new file mode 100644 index 0000000000000..d83ef2ce7817a --- /dev/null +++ b/plugins/discourse-github/spec/lib/github_pr_onebox_status_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +RSpec.describe Onebox::Engine::GithubPullRequestOnebox do + let(:gh_link) { "https://github.com/discourse/discourse/pull/1253/" } + let(:api_uri) { "https://api.github.com/repos/discourse/discourse/pulls/1253" } + let(:reviews_api_uri) { "#{api_uri}/reviews" } + + def open_pr_response + resp = MultiJson.load(onebox_response("githubpullrequest")) + resp["state"] = "open" + resp["merged"] = false + resp["draft"] = false + resp + end + + before { enable_current_plugin } + + def onebox_html + Onebox::Engine::GithubPullRequestOnebox.new(gh_link).to_html + end + + describe "PR status" do + context "when github_pr_status_enabled is false" do + before do + SiteSetting.github_pr_status_enabled = false + stub_request(:get, api_uri).to_return(status: 200, body: MultiJson.dump(open_pr_response)) + end + + it "does not include status class" do + expect(onebox_html).not_to include("--gh-status-") + end + end + + context "when github_pr_status_enabled is true" do + before { SiteSetting.github_pr_status_enabled = true } + + context "with open PR" do + before do + stub_request(:get, api_uri).to_return(status: 200, body: MultiJson.dump(open_pr_response)) + stub_request(:get, reviews_api_uri).to_return(status: 200, body: "[]") + end + + it "includes open status" do + expect(onebox_html).to include("--gh-status-open") + end + end + + context "when PR is merged" do + before do + resp = open_pr_response + resp["merged"] = true + stub_request(:get, api_uri).to_return(status: 200, body: MultiJson.dump(resp)) + end + + it "includes merged status" do + expect(onebox_html).to include("--gh-status-merged") + end + end + + context "when PR is closed" do + before do + resp = open_pr_response + resp["state"] = "closed" + stub_request(:get, api_uri).to_return(status: 200, body: MultiJson.dump(resp)) + end + + it "includes closed status" do + expect(onebox_html).to include("--gh-status-closed") + end + end + + context "when PR is draft" do + before do + resp = open_pr_response + resp["draft"] = true + stub_request(:get, api_uri).to_return(status: 200, body: MultiJson.dump(resp)) + stub_request(:get, reviews_api_uri).to_return(status: 200, body: "[]") + end + + it "includes draft status" do + expect(onebox_html).to include("--gh-status-draft") + end + end + + context "when PR is approved" do + before do + stub_request(:get, api_uri).to_return(status: 200, body: MultiJson.dump(open_pr_response)) + reviews = [ + { "user" => { "id" => 1 }, "state" => "APPROVED", "submitted_at" => Time.now.iso8601 }, + ] + stub_request(:get, reviews_api_uri).to_return(status: 200, body: MultiJson.dump(reviews)) + end + + it "includes approved status" do + expect(onebox_html).to include("--gh-status-approved") + end + end + + context "when reviews API fails" do + before do + stub_request(:get, api_uri).to_return(status: 200, body: MultiJson.dump(open_pr_response)) + stub_request(:get, reviews_api_uri).to_return(status: 500, body: "error") + end + + it "falls back gracefully without status" do + expect(onebox_html).not_to include("--gh-status-") + end + end + end + end +end diff --git a/plugins/discourse-github/spec/lib/github_pr_status_spec.rb b/plugins/discourse-github/spec/lib/github_pr_status_spec.rb deleted file mode 100644 index 291130dac5bea..0000000000000 --- a/plugins/discourse-github/spec/lib/github_pr_status_spec.rb +++ /dev/null @@ -1,243 +0,0 @@ -# frozen_string_literal: true - -describe GithubPrStatus do - let(:owner) { "discourse" } - let(:repo) { "discourse" } - let(:pr_number) { 123 } - - let(:pr_url) { "https://api.github.com/repos/#{owner}/#{repo}/pulls/#{pr_number}" } - let(:reviews_url) { "https://api.github.com/repos/#{owner}/#{repo}/pulls/#{pr_number}/reviews" } - - before { enable_current_plugin } - - def stub_pr_response(body) - stub_request(:get, pr_url).to_return(status: 200, body: body.to_json, headers: {}) - end - - def stub_reviews_response(body) - stub_request(:get, reviews_url).to_return(status: 200, body: body.to_json, headers: {}) - end - - describe ".fetch" do - it "returns 'merged' when the PR is merged" do - stub_pr_response({ "merged" => true, "state" => "closed" }) - - expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("merged") - end - - it "returns 'closed' when the PR is closed but not merged" do - stub_pr_response({ "merged" => false, "state" => "closed" }) - - expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("closed") - end - - it "returns 'draft' when the PR is a draft" do - stub_pr_response({ "merged" => false, "state" => "open", "draft" => true }) - - expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("draft") - end - - it "returns 'approved' when the PR has only approved reviews" do - stub_pr_response({ "merged" => false, "state" => "open", "draft" => false }) - stub_reviews_response( - [ - { - "user" => { - "id" => 1, - }, - "state" => "APPROVED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - ], - ) - - expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("approved") - end - - it "returns 'open' when the PR has no reviews" do - stub_pr_response({ "merged" => false, "state" => "open", "draft" => false }) - stub_reviews_response([]) - - expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("open") - end - - it "returns 'open' when the PR has changes requested" do - stub_pr_response({ "merged" => false, "state" => "open", "draft" => false }) - stub_reviews_response( - [ - { - "user" => { - "id" => 1, - }, - "state" => "CHANGES_REQUESTED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - ], - ) - - expect(GithubPrStatus.fetch(owner, repo, pr_number)).to eq("open") - end - - it "returns nil when the API request fails" do - stub_request(:get, pr_url).to_return(status: 404, body: "", headers: {}) - - expect(GithubPrStatus.fetch(owner, repo, pr_number)).to be_nil - end - - it "logs a warning when rate limited" do - reset_time = Time.now.to_i + 3600 - - stub_request(:get, pr_url).to_return( - status: 403, - body: "", - headers: { - "x-ratelimit-remaining" => "0", - "x-ratelimit-reset" => reset_time.to_s, - }, - ) - - Rails.logger.expects(:warn).with(regexp_matches(/GitHub API rate limit exceeded/)) - - Rails.logger.expects(:error).with(regexp_matches(/GitHub PR status error/)) - - expect(GithubPrStatus.fetch(owner, repo, pr_number)).to be_nil - end - end - - describe ".approved?" do - it "returns false when reviews is empty" do - expect(GithubPrStatus.send(:approved?, [])).to eq(false) - end - - it "returns false when reviews is nil" do - expect(GithubPrStatus.send(:approved?, nil)).to eq(false) - end - - it "returns true when all reviews are approved" do - reviews = [ - { - "user" => { - "id" => 1, - }, - "state" => "APPROVED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - { - "user" => { - "id" => 2, - }, - "state" => "APPROVED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - ] - expect(GithubPrStatus.send(:approved?, reviews)).to eq(true) - end - - it "returns true when reviews are approved or dismissed with at least one approval" do - reviews = [ - { - "user" => { - "id" => 1, - }, - "state" => "APPROVED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - { - "user" => { - "id" => 2, - }, - "state" => "DISMISSED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - ] - expect(GithubPrStatus.send(:approved?, reviews)).to eq(true) - end - - it "returns false when all reviews are dismissed" do - reviews = [ - { - "user" => { - "id" => 1, - }, - "state" => "DISMISSED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - ] - expect(GithubPrStatus.send(:approved?, reviews)).to eq(false) - end - - it "returns false when any review requests changes" do - reviews = [ - { - "user" => { - "id" => 1, - }, - "state" => "APPROVED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - { - "user" => { - "id" => 2, - }, - "state" => "CHANGES_REQUESTED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - ] - expect(GithubPrStatus.send(:approved?, reviews)).to eq(false) - end - - it "uses the latest review from each user" do - reviews = [ - { - "user" => { - "id" => 1, - }, - "state" => "CHANGES_REQUESTED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - { - "user" => { - "id" => 1, - }, - "state" => "APPROVED", - "submitted_at" => "2024-01-02T00:00:00Z", - }, - ] - expect(GithubPrStatus.send(:approved?, reviews)).to eq(true) - end - - it "ignores PENDING reviews" do - reviews = [ - { - "user" => { - "id" => 1, - }, - "state" => "APPROVED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - { "user" => { "id" => 2 }, "state" => "PENDING", "submitted_at" => "2024-01-02T00:00:00Z" }, - ] - expect(GithubPrStatus.send(:approved?, reviews)).to eq(true) - end - - it "ignores COMMENTED reviews" do - reviews = [ - { - "user" => { - "id" => 1, - }, - "state" => "APPROVED", - "submitted_at" => "2024-01-01T00:00:00Z", - }, - { - "user" => { - "id" => 2, - }, - "state" => "COMMENTED", - "submitted_at" => "2024-01-02T00:00:00Z", - }, - ] - expect(GithubPrStatus.send(:approved?, reviews)).to eq(true) - end - end -end diff --git a/plugins/discourse-github/spec/requests/pull_requests_controller_spec.rb b/plugins/discourse-github/spec/requests/pull_requests_controller_spec.rb deleted file mode 100644 index 9321814d6d89b..0000000000000 --- a/plugins/discourse-github/spec/requests/pull_requests_controller_spec.rb +++ /dev/null @@ -1,53 +0,0 @@ -# frozen_string_literal: true - -describe DiscourseGithub::PullRequestsController do - let(:owner) { "discourse" } - let(:repo) { "discourse" } - let(:pr_number) { 123 } - - before { enable_current_plugin } - - describe "#status" do - it "returns the PR status when successful" do - GithubPrStatus.stubs(:fetch).with(owner, repo, pr_number.to_s).returns("open") - - get "/discourse-github/#{owner}/#{repo}/pulls/#{pr_number}/status.json" - - expect(response.status).to eq(200) - expect(response.parsed_body["state"]).to eq("open") - end - - it "returns bad_gateway when GithubPrStatus returns nil" do - GithubPrStatus.stubs(:fetch).with(owner, repo, pr_number.to_s).returns(nil) - - get "/discourse-github/#{owner}/#{repo}/pulls/#{pr_number}/status.json" - - expect(response.status).to eq(502) - expect(response.parsed_body["error"]).to eq( - I18n.t("discourse_github.errors.failed_to_fetch_pr_status"), - ) - end - - %w[merged closed draft approved open].each do |state| - it "returns '#{state}' status correctly" do - GithubPrStatus.stubs(:fetch).with(owner, repo, pr_number.to_s).returns(state) - - get "/discourse-github/#{owner}/#{repo}/pulls/#{pr_number}/status.json" - - expect(response.status).to eq(200) - expect(response.parsed_body["state"]).to eq(state) - end - end - - it "handles hyphens in owner and repo names" do - special_owner = "my-org" - special_repo = "my-repo" - GithubPrStatus.stubs(:fetch).with(special_owner, special_repo, pr_number.to_s).returns("open") - - get "/discourse-github/#{special_owner}/#{special_repo}/pulls/#{pr_number}/status.json" - - expect(response.status).to eq(200) - expect(response.parsed_body["state"]).to eq("open") - end - end -end diff --git a/plugins/discourse-github/spec/requests/webhooks_controller_spec.rb b/plugins/discourse-github/spec/requests/webhooks_controller_spec.rb new file mode 100644 index 0000000000000..f1c6434dfb784 --- /dev/null +++ b/plugins/discourse-github/spec/requests/webhooks_controller_spec.rb @@ -0,0 +1,151 @@ +# frozen_string_literal: true + +RSpec.describe DiscourseGithub::WebhooksController do + let(:webhook_secret) { "test_secret_123" } + let(:pr_url) { "https://github.com/discourse/discourse/pull/123" } + + let(:pull_request_payload) do + { action: "closed", pull_request: { html_url: pr_url, merged: true } } + end + + let(:pull_request_review_payload) do + { action: "submitted", review: { state: "approved" }, pull_request: { html_url: pr_url } } + end + + def sign_payload(payload, secret) + body = payload.to_json + signature = "sha256=" + OpenSSL::HMAC.hexdigest("SHA256", secret, body) + [body, signature] + end + + before do + SiteSetting.enable_discourse_github_plugin = true + SiteSetting.github_webhook_secret = webhook_secret + end + + describe "#github" do + context "when plugin is disabled" do + before { SiteSetting.enable_discourse_github_plugin = false } + + it "returns 404" do + body, signature = sign_payload(pull_request_payload, webhook_secret) + post "/discourse-github/webhooks/github", + params: body, + headers: { + "CONTENT_TYPE" => "application/json", + "X-GitHub-Event" => "pull_request", + "X-Hub-Signature-256" => signature, + } + expect(response.status).to eq(404) + end + end + + context "with invalid signature" do + it "returns 403" do + body, _signature = sign_payload(pull_request_payload, webhook_secret) + post "/discourse-github/webhooks/github", + params: body, + headers: { + "CONTENT_TYPE" => "application/json", + "X-GitHub-Event" => "pull_request", + "X-Hub-Signature-256" => "sha256=invalid", + } + expect(response.status).to eq(403) + end + end + + context "with missing signature" do + it "returns 403" do + post "/discourse-github/webhooks/github", + params: pull_request_payload.to_json, + headers: { + "CONTENT_TYPE" => "application/json", + "X-GitHub-Event" => "pull_request", + } + expect(response.status).to eq(403) + end + end + + context "with missing webhook secret setting" do + before { SiteSetting.github_webhook_secret = "" } + + it "returns 403" do + body, signature = sign_payload(pull_request_payload, webhook_secret) + post "/discourse-github/webhooks/github", + params: body, + headers: { + "CONTENT_TYPE" => "application/json", + "X-GitHub-Event" => "pull_request", + "X-Hub-Signature-256" => signature, + } + expect(response.status).to eq(403) + end + end + + context "with valid signature" do + it "enqueues rebake job for pull_request event" do + body, signature = sign_payload(pull_request_payload, webhook_secret) + + expect_enqueued_with(job: :rebake_github_pr_posts, args: { pr_url: pr_url }) do + post "/discourse-github/webhooks/github", + params: body, + headers: { + "CONTENT_TYPE" => "application/json", + "X-GitHub-Event" => "pull_request", + "X-Hub-Signature-256" => signature, + } + end + + expect(response.status).to eq(200) + end + + it "enqueues rebake job for pull_request_review event" do + body, signature = sign_payload(pull_request_review_payload, webhook_secret) + + expect_enqueued_with(job: :rebake_github_pr_posts, args: { pr_url: pr_url }) do + post "/discourse-github/webhooks/github", + params: body, + headers: { + "CONTENT_TYPE" => "application/json", + "X-GitHub-Event" => "pull_request_review", + "X-Hub-Signature-256" => signature, + } + end + + expect(response.status).to eq(200) + end + + it "ignores other event types" do + payload = { action: "created", issue: { html_url: "https://github.com/org/repo/issues/1" } } + body, signature = sign_payload(payload, webhook_secret) + + post "/discourse-github/webhooks/github", + params: body, + headers: { + "CONTENT_TYPE" => "application/json", + "X-GitHub-Event" => "issues", + "X-Hub-Signature-256" => signature, + } + + expect(response.status).to eq(200) + expect(Jobs::RebakeGithubPrPosts.jobs.size).to eq(0) + end + + it "handles missing PR URL gracefully" do + payload = { action: "closed", pull_request: {} } + body, signature = sign_payload(payload, webhook_secret) + + post "/discourse-github/webhooks/github", + params: body, + headers: { + "CONTENT_TYPE" => "application/json", + "X-GitHub-Event" => "pull_request", + "X-Hub-Signature-256" => signature, + } + + expect(response.status).to eq(200) + expect(Jobs::RebakeGithubPrPosts.jobs.size).to eq(0) + end + end + end +end diff --git a/plugins/discourse-github/test/javascripts/acceptance/github-pr-status-test.js b/plugins/discourse-github/test/javascripts/acceptance/github-pr-status-test.js deleted file mode 100644 index a9d73e262b1b4..0000000000000 --- a/plugins/discourse-github/test/javascripts/acceptance/github-pr-status-test.js +++ /dev/null @@ -1,242 +0,0 @@ -import { visit } from "@ember/test-helpers"; -import { test } from "qunit"; -import { acceptance } from "discourse/tests/helpers/qunit-helpers"; -import { i18n } from "discourse-i18n"; - -const PR_ONEBOX_HTML = ` - -`; - -function topicWithGithubPrOnebox() { - return { - post_stream: { - posts: [ - { - id: 1, - username: "test_user", - avatar_template: "/letter_avatar_proxy/v2/letter/t/ac91a4/{size}.png", - created_at: "2024-01-01T00:00:00.000Z", - cooked: PR_ONEBOX_HTML, - post_number: 1, - post_type: 1, - updated_at: "2024-01-01T00:00:00.000Z", - reply_count: 0, - reply_to_post_number: null, - quote_count: 0, - incoming_link_count: 0, - reads: 1, - score: 0, - yours: false, - topic_id: 1, - topic_slug: "test-topic", - version: 1, - can_edit: false, - can_delete: false, - can_recover: false, - can_wiki: false, - read: true, - actions_summary: [], - moderator: false, - admin: false, - staff: false, - user_id: 2, - hidden: false, - trust_level: 1, - deleted_at: null, - user_deleted: false, - can_view_edit_history: true, - wiki: false, - }, - ], - stream: [1], - }, - timeline_lookup: [[1, 0]], - id: 1, - title: "Test Topic with GitHub PR", - fancy_title: "Test Topic with GitHub PR", - posts_count: 1, - created_at: "2024-01-01T00:00:00.000Z", - views: 1, - reply_count: 0, - participant_count: 1, - like_count: 0, - last_posted_at: "2024-01-01T00:00:00.000Z", - visible: true, - closed: false, - archived: false, - has_summary: false, - archetype: "regular", - slug: "test-topic", - category_id: 1, - word_count: 10, - deleted_at: null, - user_id: 2, - draft: null, - draft_key: "topic_1", - draft_sequence: 0, - posted: false, - pinned_globally: false, - pinned: false, - details: { - created_by: { - id: 2, - username: "test_user", - avatar_template: "/letter_avatar_proxy/v2/letter/t/ac91a4/{size}.png", - }, - last_poster: { - id: 2, - username: "test_user", - avatar_template: "/letter_avatar_proxy/v2/letter/t/ac91a4/{size}.png", - }, - participants: [ - { - id: 2, - username: "test_user", - avatar_template: "/letter_avatar_proxy/v2/letter/t/ac91a4/{size}.png", - post_count: 1, - }, - ], - notification_level: 1, - can_create_post: true, - can_reply_as_new_topic: true, - can_flag_topic: true, - }, - highest_post_number: 1, - last_read_post_number: 1, - last_read_post_id: 1, - has_deleted: false, - actions_summary: [], - chunk_size: 20, - bookmarked: false, - tags: [], - message_bus_last_id: 0, - }; -} - -acceptance("Discourse GitHub | PR Status", function (needs) { - needs.user(); - - needs.settings({ - enable_discourse_github_plugin: true, - github_pr_status_enabled: true, - }); - - needs.pretender((server, helper) => { - server.get("/t/1.json", () => helper.response(topicWithGithubPrOnebox())); - - server.get("/discourse-github/:owner/:repo/pulls/:number/status.json", () => - helper.response({ state: "merged" }) - ); - }); - - test("applies PR status class to onebox", async function (assert) { - await visit("/t/test-topic/1"); - - assert - .dom(".onebox.githubpullrequest") - .hasClass("--gh-status-merged", "onebox has merged status class"); - - assert - .dom(".onebox.githubpullrequest") - .hasAttribute( - "data-gh-pr-status", - "merged", - "onebox has data attribute with status" - ); - }); - - test("sets title on icon container", async function (assert) { - await visit("/t/test-topic/1"); - - assert - .dom(".onebox.githubpullrequest .github-icon-container") - .hasAttribute( - "title", - i18n("github.pr_status.merged"), - "icon container has correct title" - ); - }); -}); - -acceptance("Discourse GitHub | PR Status - Different States", function (needs) { - needs.user(); - - needs.settings({ - enable_discourse_github_plugin: true, - github_pr_status_enabled: true, - }); - - needs.pretender((server, helper) => { - server.get("/t/1.json", () => helper.response(topicWithGithubPrOnebox())); - - server.get("/discourse-github/:owner/:repo/pulls/:number/status.json", () => - helper.response({ state: "open" }) - ); - }); - - test("applies open status class", async function (assert) { - await visit("/t/test-topic/1"); - - assert - .dom(".onebox.githubpullrequest") - .hasClass("--gh-status-open", "onebox has open status class"); - }); -}); - -acceptance("Discourse GitHub | PR Status - Disabled", function (needs) { - needs.user(); - - needs.settings({ - enable_discourse_github_plugin: true, - github_pr_status_enabled: false, - }); - - needs.pretender((server, helper) => { - server.get("/t/1.json", () => helper.response(topicWithGithubPrOnebox())); - }); - - test("does not fetch status when disabled", async function (assert) { - await visit("/t/test-topic/1"); - - assert - .dom(".onebox.githubpullrequest") - .doesNotHaveClass( - "--gh-status-merged", - "onebox does not have status class when disabled" - ); - - assert - .dom(".onebox.githubpullrequest[data-gh-pr-status]") - .doesNotExist("onebox does not have data attribute when disabled"); - }); -}); - -acceptance("Discourse GitHub | PR Status - Anonymous User", function (needs) { - needs.settings({ - enable_discourse_github_plugin: true, - github_pr_status_enabled: true, - }); - - needs.pretender((server, helper) => { - server.get("/t/1.json", () => helper.response(topicWithGithubPrOnebox())); - }); - - test("does not fetch status for anonymous users", async function (assert) { - await visit("/t/test-topic/1"); - - assert - .dom(".onebox.githubpullrequest[data-gh-pr-status]") - .doesNotExist("onebox does not have data attribute for anonymous users"); - }); -}); From d6037202902a3b494778d7f735f8de1e4070ba0a Mon Sep 17 00:00:00 2001 From: zogstrip Date: Thu, 4 Dec 2025 19:50:43 +0100 Subject: [PATCH 4/4] feeeedback --- .../engine/github_pull_request_onebox.rb | 3 ++ plugins/chat/app/models/chat/message_link.rb | 39 +++++++++++-------- ...0251203000001_create_chat_message_links.rb | 4 -- .../jobs/regular/chat/process_message_spec.rb | 8 ++++ .../spec/models/chat/message_link_spec.rb | 3 -- .../discourse_github/webhooks_controller.rb | 1 + .../config/locales/server.en.yml | 2 +- .../spec/lib/github_pr_onebox_status_spec.rb | 23 +++++++++++ 8 files changed, 59 insertions(+), 24 deletions(-) diff --git a/lib/onebox/engine/github_pull_request_onebox.rb b/lib/onebox/engine/github_pull_request_onebox.rb index 813918387d4a8..b70263b9a3677 100644 --- a/lib/onebox/engine/github_pull_request_onebox.rb +++ b/lib/onebox/engine/github_pull_request_onebox.rb @@ -112,6 +112,9 @@ def load_json(url) ::MultiJson.load( URI.parse(url).open({ read_timeout: timeout }.merge(github_auth_header(match[:org]))), ) + rescue OpenURI::HTTPError => e + Rails.logger.warn("GitHub API error: #{e.io.status[0]} fetching #{url}") + raise end def fetch_pr_status(pr_data) diff --git a/plugins/chat/app/models/chat/message_link.rb b/plugins/chat/app/models/chat/message_link.rb index 19ddd55e89d1b..6cec45885f625 100644 --- a/plugins/chat/app/models/chat/message_link.rb +++ b/plugins/chat/app/models/chat/message_link.rb @@ -5,13 +5,11 @@ class MessageLink < ActiveRecord::Base self.table_name = "chat_message_links" belongs_to :chat_message, class_name: "Chat::Message" - belongs_to :chat_channel, class_name: "Chat::Channel" - belongs_to :user validates :url, presence: true, length: { maximum: 500 } def self.extract_from(message) - return if message.blank? || message.user_id.blank? || message.deleted_at.present? + return if message.blank? || message.deleted_at.present? current_urls = [] @@ -26,28 +24,20 @@ def self.extract_from(message) next if parsed.host.blank? || parsed.host.length > 100 current_urls << url - upsert_link(message, url, parsed.host) + upsert_link(message, url) end cleanup_entries(message, current_urls) end - def self.upsert_link(message, url, domain) + def self.upsert_link(message, url) sql = <<~SQL - INSERT INTO chat_message_links (chat_message_id, chat_channel_id, user_id, url, domain, created_at, updated_at) - VALUES (:chat_message_id, :chat_channel_id, :user_id, :url, :domain, :now, :now) + INSERT INTO chat_message_links (chat_message_id, url, created_at, updated_at) + VALUES (:chat_message_id, :url, :now, :now) ON CONFLICT (chat_message_id, url) DO NOTHING SQL - DB.exec( - sql, - chat_message_id: message.id, - chat_channel_id: message.chat_channel_id, - user_id: message.user_id, - url: url, - domain: domain, - now: Time.current, - ) + DB.exec(sql, chat_message_id: message.id, url: url, now: Time.current) end def self.cleanup_entries(message, current_urls) @@ -63,3 +53,20 @@ def self.cleanup_entries(message, current_urls) end end end + +# == Schema Information +# +# Table name: chat_message_links +# +# id :bigint not null, primary key +# url :string(500) not null +# created_at :datetime not null +# updated_at :datetime not null +# chat_message_id :bigint not null +# +# Indexes +# +# index_chat_message_links_on_chat_message_id (chat_message_id) +# index_chat_message_links_on_url (url) +# unique_chat_message_links (chat_message_id,url) UNIQUE +# diff --git a/plugins/chat/db/migrate/20251203000001_create_chat_message_links.rb b/plugins/chat/db/migrate/20251203000001_create_chat_message_links.rb index 64d2ca95a8a87..82158158100d7 100644 --- a/plugins/chat/db/migrate/20251203000001_create_chat_message_links.rb +++ b/plugins/chat/db/migrate/20251203000001_create_chat_message_links.rb @@ -4,15 +4,11 @@ class CreateChatMessageLinks < ActiveRecord::Migration[7.2] def change create_table :chat_message_links do |t| t.bigint :chat_message_id, null: false - t.bigint :chat_channel_id, null: false - t.integer :user_id, null: false t.string :url, null: false, limit: 500 - t.string :domain, limit: 100 t.timestamps end add_index :chat_message_links, :chat_message_id - add_index :chat_message_links, :chat_channel_id add_index :chat_message_links, :url add_index :chat_message_links, %i[chat_message_id url], diff --git a/plugins/chat/spec/jobs/regular/chat/process_message_spec.rb b/plugins/chat/spec/jobs/regular/chat/process_message_spec.rb index d83491e845ca6..392d2fd9a5f53 100644 --- a/plugins/chat/spec/jobs/regular/chat/process_message_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat/process_message_spec.rb @@ -31,4 +31,12 @@ chat_message.destroy expect { described_class.new.execute(chat_message_id: chat_message.id) }.not_to raise_exception end + + it "extracts links from the message" do + described_class.new.execute(chat_message_id: chat_message.id) + + link = Chat::MessageLink.find_by(chat_message_id: chat_message.id) + expect(link).to be_present + expect(link.url).to eq("https://discourse.org/team") + end end diff --git a/plugins/chat/spec/models/chat/message_link_spec.rb b/plugins/chat/spec/models/chat/message_link_spec.rb index 9d51b505dd59f..0f6985d59f094 100644 --- a/plugins/chat/spec/models/chat/message_link_spec.rb +++ b/plugins/chat/spec/models/chat/message_link_spec.rb @@ -20,10 +20,7 @@ expect(Chat::MessageLink.count).to eq(1) link = Chat::MessageLink.first expect(link.url).to eq("https://github.com/discourse/discourse/pull/123") - expect(link.domain).to eq("github.com") expect(link.chat_message_id).to eq(message.id) - expect(link.chat_channel_id).to eq(channel.id) - expect(link.user_id).to eq(user.id) end it "extracts multiple links from a message" do diff --git a/plugins/discourse-github/app/controllers/discourse_github/webhooks_controller.rb b/plugins/discourse-github/app/controllers/discourse_github/webhooks_controller.rb index 94988e977bb20..e36c69f5dce82 100644 --- a/plugins/discourse-github/app/controllers/discourse_github/webhooks_controller.rb +++ b/plugins/discourse-github/app/controllers/discourse_github/webhooks_controller.rb @@ -31,6 +31,7 @@ def verify_signature signature = request.headers["X-Hub-Signature-256"] return false if signature.blank? + request.body.rewind body = request.body.read expected = "sha256=" + OpenSSL::HMAC.hexdigest("SHA256", secret, body) diff --git a/plugins/discourse-github/config/locales/server.en.yml b/plugins/discourse-github/config/locales/server.en.yml index f6ce1856869ed..d397d3e582fd5 100644 --- a/plugins/discourse-github/config/locales/server.en.yml +++ b/plugins/discourse-github/config/locales/server.en.yml @@ -6,7 +6,7 @@ en: github_linkback_access_token: 'A valid access token for the user who will post the linkback and for counting commits/contributions to grant badges. See here for instructions on how to get a token.' github_linkback_maximum_links: "Maximum number of linkbacks to create from one single post. When a post contains more links, none is created." github_permalinks_enabled: "Enable GitHub permalink overwrites" - github_pr_status_enabled: "Show GitHub pull request status on oneboxes (draft, open, approved, changes requested, merged, closed)" + github_pr_status_enabled: "Show GitHub pull request status on oneboxes (draft, open, approved, merged, closed)" github_webhook_secret: "Secret token for GitHub webhook signature verification. Configure this value in your GitHub webhook settings." github_permalinks_exclude: "Filename or directory that should be excluded from permalink overwrites. Provide only filename or full path: user/repository/optional-directory/filename" github_badges_enabled: "Enable GitHub badges" diff --git a/plugins/discourse-github/spec/lib/github_pr_onebox_status_spec.rb b/plugins/discourse-github/spec/lib/github_pr_onebox_status_spec.rb index d83ef2ce7817a..e89014c9d00fa 100644 --- a/plugins/discourse-github/spec/lib/github_pr_onebox_status_spec.rb +++ b/plugins/discourse-github/spec/lib/github_pr_onebox_status_spec.rb @@ -50,6 +50,7 @@ def onebox_html resp = open_pr_response resp["merged"] = true stub_request(:get, api_uri).to_return(status: 200, body: MultiJson.dump(resp)) + stub_request(:get, reviews_api_uri).to_return(status: 200, body: "[]") end it "includes merged status" do @@ -62,6 +63,7 @@ def onebox_html resp = open_pr_response resp["state"] = "closed" stub_request(:get, api_uri).to_return(status: 200, body: MultiJson.dump(resp)) + stub_request(:get, reviews_api_uri).to_return(status: 200, body: "[]") end it "includes closed status" do @@ -96,6 +98,27 @@ def onebox_html end end + context "when PR has changes requested" do + before do + stub_request(:get, api_uri).to_return(status: 200, body: MultiJson.dump(open_pr_response)) + reviews = [ + { + "user" => { + "id" => 1, + }, + "state" => "CHANGES_REQUESTED", + "submitted_at" => Time.now.iso8601, + }, + ] + stub_request(:get, reviews_api_uri).to_return(status: 200, body: MultiJson.dump(reviews)) + end + + it "shows as open status (not approved)" do + expect(onebox_html).to include("--gh-status-open") + expect(onebox_html).not_to include("--gh-status-approved") + end + end + context "when reviews API fails" do before do stub_request(:get, api_uri).to_return(status: 200, body: MultiJson.dump(open_pr_response))