Merge pull request #1646 from ehuss/rfc-rendered

Fix RFC rendered linkifier.

Author: Mark-Simulacrum

src/github.rs

@@ -231,39 +231,63 @@ pub struct Label {
     pub name: String,
 }
 
-#[derive(Debug, serde::Deserialize)]
-pub struct PullRequestDetails {
-    // none for now
-}
-
+/// An issue or pull request.
+///
+/// For convenience, since issues and pull requests share most of their
+/// fields, this struct is used for both. The `pull_request` field can be used
+/// to determine which it is. Some fields are only available on pull requests
+/// (but not always, check the GitHub API for details).
 #[derive(Debug, serde::Deserialize)]
 pub struct Issue {
     pub number: u64,
     #[serde(deserialize_with = "opt_string")]
     pub body: String,
     created_at: chrono::DateTime<Utc>,
     pub updated_at: chrono::DateTime<Utc>,
+    /// The SHA for a merge commit.
+    ///
+    /// This field is complicated, see the [Pull Request
+    /// docs](https://docs.github.com/en/rest/pulls/pulls#get-a-pull-request)
+    /// for details.
     #[serde(default)]
     pub merge_commit_sha: Option<String>,
     pub title: String,
+    /// The common URL for viewing this issue or PR.
+    ///
+    /// Example: `https://github.com/octocat/Hello-World/pull/1347`
     pub html_url: String,
     pub user: User,
     pub labels: Vec<Label>,
     pub assignees: Vec<User>,
-    pub pull_request: Option<PullRequestDetails>,
+    /// This is true if this is a pull request.
+    ///
+    /// Note that this field does not come from GitHub. This is manually added
+    /// when the webhook arrives to help differentiate between an event
+    /// related to an issue versus a pull request.
+    #[serde(default)]
+    pub pull_request: bool,
+    /// Whether or not the pull request was merged.
     #[serde(default)]
     pub merged: bool,
     #[serde(default)]
     pub draft: bool,
-    // API URL
+    /// The API URL for discussion comments.
+    ///
+    /// Example: `https://api.github.com/repos/octocat/Hello-World/issues/1347/comments`
     comments_url: String,
+    /// The repository for this issue.
+    ///
+    /// Note that this is constructed via the [`Issue::repository`] method.
+    /// It is not deserialized from the GitHub API.
     #[serde(skip)]
     repository: OnceCell<IssueRepository>,
 
+    /// The base commit for a PR (the branch of the destination repo).
     #[serde(default)]
-    base: Option<CommitBase>,
+    pub base: Option<CommitBase>,
+    /// The head commit for a PR (the branch from the source repo).
     #[serde(default)]
-    head: Option<CommitBase>,
+    pub head: Option<CommitBase>,
 }
 
 /// Contains only the parts of `Issue` that are needed for turning the issue title into a Zulip
@@ -431,7 +455,7 @@ impl Issue {
     }
 
     pub fn is_pr(&self) -> bool {
-        self.pull_request.is_some()
+        self.pull_request
     }
 
     pub async fn get_comment(&self, client: &GithubClient, id: usize) -> anyhow::Result<Comment> {
@@ -798,6 +822,9 @@ pub enum PullRequestReviewAction {
     Dismissed,
 }
 
+/// A pull request review event.
+///
+/// <https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#pull_request_review>
 #[derive(Debug, serde::Deserialize)]
 pub struct PullRequestReviewEvent {
     pub action: PullRequestReviewAction,
@@ -877,6 +904,9 @@ struct PullRequestEventFields {}
 #[derive(Clone, Debug, serde::Deserialize)]
 pub struct CommitBase {
     sha: String,
+    #[serde(rename = "ref")]
+    pub git_ref: String,
+    pub repo: Repository,
 }
 
 pub fn files_changed(diff: &str) -> Vec<&str> {
@@ -1215,11 +1245,24 @@ pub struct PushEvent {
     sender: User,
 }
 
+/// An event triggered by a webhook.
 #[derive(Debug)]
 pub enum Event {
+    /// A Git branch or tag is created.
     Create(CreateEvent),
+    /// A comment on an issue or PR.
+    ///
+    /// Can be:
+    /// - Regular comment on an issue or PR.
+    /// - A PR review.
+    /// - A comment on a PR review.
+    ///
+    /// These different scenarios are unified into the `IssueComment` variant
+    /// when triagebot receives the corresponding webhook event.
     IssueComment(IssueCommentEvent),
+    /// Activity on an issue or PR.
     Issue(IssuesEvent),
+    /// One or more commits are pushed to a repository branch or tag.
     Push(PushEvent),
 }
 

src/handlers/review_submitted.rs

@@ -10,8 +10,7 @@ pub(crate) async fn handle(
         event @ IssueCommentEvent {
             action: IssueCommentAction::Created,
             issue: Issue {
-                pull_request: Some(_),
-                ..
+                pull_request: true, ..
             },
             ..
         },

src/handlers/rfc_helper.rs

@@ -23,15 +23,29 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> {
 }
 
 async fn add_rendered_link(ctx: &Context, e: &IssuesEvent) -> anyhow::Result<()> {
-    if e.action == IssuesAction::Opened || e.action == IssuesAction::Synchronize {
+    if e.action == IssuesAction::Opened {
         let files = e.issue.files(&ctx.github).await?;
 
         if let Some(file) = files.iter().find(|f| f.filename.starts_with("text/")) {
             if !e.issue.body.contains("[Rendered]") {
+                // This URL should be stable while the PR is open, even if the
+                // user pushes new commits.
+                //
+                // It will go away if the user deletes their branch, or if
+                // they reset it (such as if they created a PR from master).
+                // That should usually only happen after the PR is closed.
+                // During the closing process, the closer should update the
+                // Rendered link to the new location (which we should
+                // automate!).
+                let head = e.issue.head.as_ref().unwrap();
+                let url = format!(
+                    "https://github.com/{}/blob/{}/{}",
+                    head.repo.full_name, head.git_ref, file.filename
+                );
                 e.issue
                     .edit_body(
                         &ctx.github,
-                        &format!("{}\n\n[Rendered]({})", e.issue.body, file.blob_url),
+                        &format!("{}\n\n[Rendered]({})", e.issue.body, url),
                     )
                     .await?;
             }

src/lib.rs

@@ -9,8 +9,6 @@ use interactions::ErrorComment;
 use std::fmt;
 use tracing as log;
 
-use crate::github::PullRequestDetails;
-
 pub mod actions;
 pub mod agenda;
 mod changelogs;
@@ -28,15 +26,52 @@ mod team_data;
 pub mod triage;
 pub mod zulip;
 
+/// The name of a webhook event.
 #[derive(Debug)]
 pub enum EventName {
+    /// Pull request activity.
+    ///
+    /// This gets translated to [`github::Event::Issue`] when sent to a handler.
+    ///
+    /// <https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#pull_request>
     PullRequest,
+    /// Pull request review activity.
+    ///
+    /// This gets translated to [`github::Event::IssueComment`] when sent to a handler.
+    ///
+    /// <https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#pull_request_review>
     PullRequestReview,
+    /// A comment on a pull request review.
+    ///
+    /// This gets translated to [`github::Event::IssueComment`] when sent to a handler.
+    ///
+    /// <https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#pull_request_review_comment>
     PullRequestReviewComment,
+    /// An issue or PR comment.
+    ///
+    /// This gets translated to [`github::Event::IssueComment`] when sent to a handler.
+    ///
+    /// <https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#issue_comment>
     IssueComment,
+    /// Issue activity.
+    ///
+    /// This gets translated to [`github::Event::Issue`] when sent to a handler.
+    ///
+    /// <https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#issues>
     Issue,
+    /// One or more commits are pushed to a repository branch or tag.
+    ///
+    /// This gets translated to [`github::Event::Push`] when sent to a handler.
+    ///
+    /// <https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#push>
     Push,
+    /// A Git branch or tag is created.
+    ///
+    /// This gets translated to [`github::Event::Create`] when sent to a handler.
+    ///
+    /// <https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#create>
     Create,
+    /// All other unhandled webhooks.
     Other,
 }
 
@@ -108,12 +143,7 @@ pub async fn webhook(
                 .map_err(anyhow::Error::from)?;
 
             log::info!("handling pull request review comment {:?}", payload);
-
-            // Github doesn't send a pull_request field nested into the
-            // pull_request field, so we need to adjust the deserialized result
-            // to preserve that this event came from a pull request (since it's
-            // a PR review, that's obviously the case).
-            payload.pull_request.pull_request = Some(PullRequestDetails {});
+            payload.pull_request.pull_request = true;
 
             // Treat pull request review comments exactly like pull request
             // review comments.
@@ -138,11 +168,7 @@ pub async fn webhook(
                 .context("PullRequestReview(Comment) failed to deserialize")
                 .map_err(anyhow::Error::from)?;
 
-            // Github doesn't send a pull_request field nested into the
-            // pull_request field, so we need to adjust the deserialized result
-            // to preserve that this event came from a pull request (since it's
-            // a PR review, that's obviously the case).
-            payload.issue.pull_request = Some(PullRequestDetails {});
+            payload.issue.pull_request = true;
 
             log::info!("handling pull request review comment {:?}", payload);
 
@@ -166,10 +192,14 @@ pub async fn webhook(
             github::Event::IssueComment(payload)
         }
         EventName::Issue | EventName::PullRequest => {
-            let payload = deserialize_payload::<github::IssuesEvent>(&payload)
+            let mut payload = deserialize_payload::<github::IssuesEvent>(&payload)
                 .context(format!("{:?} failed to deserialize", event))
                 .map_err(anyhow::Error::from)?;
 
+            if matches!(event, EventName::PullRequest) {
+                payload.issue.pull_request = true;
+            }
+
             log::info!("handling issue event {:?}", payload);
 
             github::Event::Issue(payload)