Merge pull request #360 from Kobzol/delegate-comment

Improve delegate comments

Author: Kobzol

src/bin/bors.rs

@@ -137,7 +137,7 @@ fn try_main(opts: Opts) -> anyhow::Result<()> {
 
     let db = Arc::new(db);
     let ctx = BorsContext::new(
-        CommandParser::new(opts.cmd_prefix.clone()),
+        CommandParser::new(opts.cmd_prefix.clone().into()),
         db.clone(),
         repos.clone(),
     );
@@ -194,7 +194,7 @@ fn try_main(opts: Opts) -> anyhow::Result<()> {
         WebhookSecret::new(opts.webhook_secret),
         repos,
         db,
-        opts.cmd_prefix,
+        opts.cmd_prefix.into(),
     );
     let server_process = webhook_server(state);
 

src/bors/command/mod.rs

@@ -1,5 +1,6 @@
 mod parser;
 use std::fmt;
+use std::fmt::{Display, Formatter};
 use std::str::FromStr;
 
 use crate::{database::DelegatedPermission, github::CommitSha};
@@ -8,6 +9,27 @@ pub use parser::{CommandParseError, CommandParser};
 /// Priority of a commit.
 pub type Priority = u32;
 
+/// Command prefix used to identify bors commands, e.g. `@bors`.
+pub struct CommandPrefix(String);
+
+impl From<String> for CommandPrefix {
+    fn from(value: String) -> Self {
+        Self(value)
+    }
+}
+
+impl Display for CommandPrefix {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
+impl AsRef<str> for CommandPrefix {
+    fn as_ref(&self) -> &str {
+        self.0.as_str()
+    }
+}
+
 /// Type of parent allowed in a try build
 #[derive(Clone, Debug, PartialEq)]
 pub enum Parent {

src/bors/command/parser.rs

@@ -1,6 +1,6 @@
 //! Defines parsers for bors commands.
 
-use crate::bors::command::{Approver, BorsCommand, Parent};
+use crate::bors::command::{Approver, BorsCommand, CommandPrefix, Parent};
 use crate::database::DelegatedPermission;
 use crate::github::CommitSha;
 use pulldown_cmark::{Event, Parser, Tag, TagEnd, TextMergeStream};
@@ -27,17 +27,17 @@ enum CommandPart<'a> {
 }
 
 pub struct CommandParser {
-    prefix: String,
+    prefix: CommandPrefix,
 }
 
 impl CommandParser {
-    pub fn new(prefix: String) -> Self {
+    pub fn new(prefix: CommandPrefix) -> Self {
         Self { prefix }
     }
 
     /// Prefix of the bot, used to invoke commands from PR comments.
     /// For example `@bors`.
-    pub fn prefix(&self) -> &str {
+    pub fn prefix(&self) -> &CommandPrefix {
         &self.prefix
     }
 
@@ -49,9 +49,9 @@ impl CommandParser {
         let segments = extract_text_from_markdown(text);
         segments
             .lines()
-            .filter_map(|line| match line.find(&self.prefix) {
+            .filter_map(|line| match line.find(self.prefix.as_ref()) {
                 Some(index) => {
-                    let input = &line[index + self.prefix.len()..];
+                    let input = &line[index + self.prefix.as_ref().len()..];
                     parse_command(input)
                 }
                 None => None,
@@ -1346,6 +1346,6 @@ I am markdown HTML comment
     }
 
     fn parse_commands(text: &str) -> Vec<Result<BorsCommand, CommandParseError>> {
-        CommandParser::new("@bors".to_string()).parse_commands(text)
+        CommandParser::new("@bors".to_string().into()).parse_commands(text)
     }
 }

src/bors/comment.rs

@@ -1,5 +1,6 @@
 use serde::Serialize;
 
+use crate::bors::command::CommandPrefix;
 use crate::database::BuildModel;
 use crate::{
     database::{WorkflowModel, WorkflowStatus},
@@ -105,7 +106,7 @@ pub fn workflow_failed_comment(workflows: &[WorkflowModel]) -> Comment {
 pub fn try_build_started_comment(
     head_sha: &CommitSha,
     merge_sha: &CommitSha,
-    bot_prefix: &str,
+    bot_prefix: &CommandPrefix,
     cancelled_workflow_urls: Vec<String>,
 ) -> Comment {
     use std::fmt::Write;
@@ -167,6 +168,26 @@ pub fn approve_non_open_pr_comment() -> Comment {
     Comment::new("Only open, non-draft PRs can be approved".to_string())
 }
 
+pub fn delegate_try_builds_comment(delegatee: &str, bot_prefix: &CommandPrefix) -> Comment {
+    Comment::new(format!(
+        r":v: @{delegatee}, you can now perform try builds on this pull request!
+
+You can now post `{bot_prefix} try` to start a try build.
+    ",
+    ))
+}
+
+/// `delegatee` is the user who received the delegation privileges, while `delegator` is the user
+/// who gave these privileges to the `delegatee`.
+pub fn delegate_comment(delegatee: &str, delegator: &str, bot_prefix: &CommandPrefix) -> Comment {
+    Comment::new(format!(
+        r#":v: @{delegatee}, you can now approve this pull request!
+
+If @{delegator} told you to "`r=me`" after making some further change, then please make that change and post `{bot_prefix} r={delegator}`.
+"#
+    ))
+}
+
 fn list_workflows_status(workflows: &[WorkflowModel]) -> String {
     workflows
         .iter()

src/bors/handlers/mod.rs

@@ -446,9 +446,16 @@ async fn handle_comment(
                     }
                     BorsCommand::SetDelegate(delegate_type) => {
                         let span = tracing::info_span!("Delegate");
-                        command_delegate(repo, database, &pr, &comment.author, delegate_type)
-                            .instrument(span)
-                            .await
+                        command_delegate(
+                            repo,
+                            database,
+                            &pr,
+                            &comment.author,
+                            delegate_type,
+                            ctx.parser.prefix(),
+                        )
+                        .instrument(span)
+                        .await
                     }
                     BorsCommand::Undelegate => {
                         let span = tracing::info_span!("Undelegate");

src/bors/handlers/review.rs

@@ -2,9 +2,11 @@ use std::sync::Arc;
 
 use crate::PgDbClient;
 use crate::bors::RepositoryState;
-use crate::bors::command::Approver;
 use crate::bors::command::RollupMode;
-use crate::bors::comment::approve_non_open_pr_comment;
+use crate::bors::command::{Approver, CommandPrefix};
+use crate::bors::comment::{
+    approve_non_open_pr_comment, delegate_comment, delegate_try_builds_comment,
+};
 use crate::bors::handlers::has_permission;
 use crate::bors::handlers::labels::handle_label_trigger;
 use crate::bors::handlers::{PullRequestData, deny_request};
@@ -102,6 +104,7 @@ pub(super) async fn command_delegate(
     pr: &PullRequestData,
     author: &GithubUser,
     delegated_permission: DelegatedPermission,
+    bot_prefix: &CommandPrefix,
 ) -> anyhow::Result<()> {
     tracing::info!(
         "Delegating PR {} {} permissions",
@@ -118,7 +121,9 @@ pub(super) async fn command_delegate(
         &repo_state,
         pr.number(),
         &pr.github.author.username,
+        &author.username,
         delegated_permission,
+        bot_prefix,
     )
     .await
 }
@@ -268,19 +273,16 @@ async fn notify_of_delegation(
     repo: &RepositoryState,
     pr_number: PullRequestNumber,
     delegatee: &str,
+    delegator: &str,
     delegated_permission: DelegatedPermission,
+    bot_prefix: &CommandPrefix,
 ) -> anyhow::Result<()> {
-    let message = match delegated_permission {
-        DelegatedPermission::Try => format!(
-            "@{} can now perform try builds on this pull request",
-            delegatee
-        ),
-        DelegatedPermission::Review => format!("@{} can now approve this pull request", delegatee),
+    let comment = match delegated_permission {
+        DelegatedPermission::Try => delegate_try_builds_comment(delegatee, bot_prefix),
+        DelegatedPermission::Review => delegate_comment(delegatee, delegator, bot_prefix),
     };
 
-    repo.client
-        .post_comment(pr_number, Comment::new(message))
-        .await
+    repo.client.post_comment(pr_number, comment).await
 }
 
 #[cfg(test)]
@@ -542,7 +544,11 @@ mod tests {
                     .await?;
                 insta::assert_snapshot!(
                     tester.get_comment().await?,
-                    @"@default-user can now approve this pull request"
+                    @r#"
+                :v: @default-user, you can now approve this pull request!
+
+                If @reviewer told you to "`r=me`" after making some further change, then please make that change and post `@bors r=reviewer`.
+                "#
                 );
 
                 tester
@@ -980,7 +986,11 @@ mod tests {
                     .await?;
                 insta::assert_snapshot!(
                     tester.get_comment().await?,
-                    @"@default-user can now perform try builds on this pull request"
+                    @r"
+                :v: @default-user, you can now perform try builds on this pull request!
+
+                You can now post `@bors try` to start a try build.
+                "
                 );
                 tester
                     .default_pr()

src/bors/handlers/trybuild.rs

@@ -2,7 +2,7 @@ use std::sync::Arc;
 
 use crate::PgDbClient;
 use crate::bors::RepositoryState;
-use crate::bors::command::Parent;
+use crate::bors::command::{CommandPrefix, Parent};
 use crate::bors::comment::no_try_build_in_progress_comment;
 use crate::bors::comment::try_build_cancelled_comment;
 use crate::bors::comment::unclean_try_build_cancelled_comment;
@@ -51,7 +51,7 @@ pub(super) async fn command_try_build(
     author: &GithubUser,
     parent: Option<Parent>,
     jobs: Vec<String>,
-    bot_prefix: &str,
+    bot_prefix: &CommandPrefix,
 ) -> anyhow::Result<()> {
     let repo = repo.as_ref();
     if !has_permission(repo, author, pr, PermissionType::Try).await? {

src/bors/mod.rs

@@ -25,6 +25,8 @@ mod handlers;
 pub mod merge_queue;
 pub mod mergeable_queue;
 
+pub use command::CommandPrefix;
+
 #[cfg(test)]
 pub static WAIT_FOR_CANCEL_TIMED_OUT_BUILDS_REFRESH: TestSyncMarker = TestSyncMarker::new();
 

src/github/server.rs

@@ -5,7 +5,7 @@ use crate::bors::mergeable_queue::{
     handle_mergeable_queue_item,
 };
 use crate::bors::{
-    BorsContext, RepositoryState, RollupMode, handle_bors_global_event,
+    BorsContext, CommandPrefix, RepositoryState, RollupMode, handle_bors_global_event,
     handle_bors_repository_event,
 };
 use crate::github::webhook::GitHubWebhook;
@@ -42,7 +42,7 @@ pub struct ServerState {
     webhook_secret: WebhookSecret,
     repositories: HashMap<GithubRepoName, Arc<RepositoryState>>,
     db: Arc<PgDbClient>,
-    cmd_prefix: String,
+    cmd_prefix: CommandPrefix,
 }
 
 impl ServerState {
@@ -52,7 +52,7 @@ impl ServerState {
         webhook_secret: WebhookSecret,
         repositories: HashMap<GithubRepoName, Arc<RepositoryState>>,
         db: Arc<PgDbClient>,
-        cmd_prefix: String,
+        cmd_prefix: CommandPrefix,
     ) -> Self {
         Self {
             repository_event_queue,
@@ -68,7 +68,7 @@ impl ServerState {
         &self.webhook_secret
     }
 
-    pub fn get_cmd_prefix(&self) -> &str {
+    pub fn get_cmd_prefix(&self) -> &CommandPrefix {
         &self.cmd_prefix
     }
 }
@@ -123,7 +123,7 @@ async fn help_handler(State(state): State<ServerStateRef>) -> impl IntoResponse
 
     HtmlTemplate(HelpTemplate {
         repos,
-        cmd_prefix: state.get_cmd_prefix().to_string(),
+        cmd_prefix: state.get_cmd_prefix().as_ref().to_string(),
     })
 }
 

src/tests/mocks/bors.rs

@@ -19,8 +19,8 @@ use super::repository::PullRequest;
 use crate::bors::merge_queue::MergeQueueSender;
 use crate::bors::mergeable_queue::MergeableQueueSender;
 use crate::bors::{
-    RollupMode, WAIT_FOR_CANCEL_TIMED_OUT_BUILDS_REFRESH, WAIT_FOR_MERGEABILITY_STATUS_REFRESH,
-    WAIT_FOR_PR_STATUS_REFRESH, WAIT_FOR_WORKFLOW_STARTED,
+    CommandPrefix, RollupMode, WAIT_FOR_CANCEL_TIMED_OUT_BUILDS_REFRESH,
+    WAIT_FOR_MERGEABILITY_STATUS_REFRESH, WAIT_FOR_PR_STATUS_REFRESH, WAIT_FOR_WORKFLOW_STARTED,
 };
 use crate::database::{BuildStatus, DelegatedPermission, OctocrabMergeableState, PullRequestModel};
 use crate::github::api::load_repositories;
@@ -33,9 +33,6 @@ use crate::tests::mocks::workflow::{
 };
 use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus};
 
-pub fn default_cmd_prefix() -> String {
-    "@bors".to_string()
-}
 use crate::tests::mocks::{
     Branch, ExternalHttpMock, GitHubState, Repo, User, default_pr_number, default_repo_name,
 };
@@ -46,6 +43,10 @@ use crate::{
     create_app, create_bors_process,
 };
 
+pub fn default_cmd_prefix() -> CommandPrefix {
+    "@bors".to_string().into()
+}
+
 pub struct BorsBuilder {
     github: GitHubState,
     pool: PgPool,
@@ -131,7 +132,7 @@ impl BorsTester {
         }
 
         let ctx = BorsContext::new(
-            CommandParser::new("@bors".to_string()),
+            CommandParser::new("@bors".to_string().into()),
             db.clone(),
             repos.clone(),
         );