refactor: cover the edit/answer surface in MediaSender, test the button flows

docs/architecture-refactor.md §3 sketched the trait with "按需扩展： edit_message_caption / delete_message / answer_callback_query …", but only the five send methods landed, so `callback.rs` and the edit-before-forward caption swap were stuck on the concrete `Bot` and remained untested (AGENTS.md still lists callback.rs as untestable). - `MediaSender` gains `answer_callback_query`, `edit_message_caption` (HTML parse mode baked in, every caller uses it) and `delete_message`; the mock records call order plus the texts, captions and answer toasts, so tests can assert what the user saw. - `send_message` now returns the sent message id instead of the whole `Message`: the only consumer of the value is the edit-before-forward prompt (which keys its record by it), and returning a `Message` forced every mock to build a teloxide type. `reply`/`reply_html` follow. - `callback.rs`: the dptree entry only unpacks the update; `handle_callback` takes plain values + `&AppContext`. `handlers/mod.rs::edit_message_handler` likewise takes the values the reply carries. Admin/setup APIs (`get_chat`, `get_chat_administrators`, `get_me`, `set_my_commands`) stay on the concrete `Bot`: they are not user flows worth a trait. - The scripted mock moves to `parking_lot::Mutex` (no poisoning unwraps). Tests: +11 (template button, forward ok/no-channel/retryable, expired+unknown prompt, caption swap via template, escaping of user text into the caption, failed swap still consuming the reply, prompt record written by post_send). fmt/clippy clean, 70 + 69 tests pass.
2026-09-23 23:32:05 +00:00 · 2026-09-17 01:31:30 +08:00
parent c2d7c8406e
commit 0a9ff58a69
5 changed files with 576 additions and 108 deletions
@@ -1,56 +1,76 @@
 //! Callback query handling: the edit-before-forward prompt's `"forward"` and
 //! `"template|<name>"` buttons.
+//!
+//! [`callback_query_handler`] is the dptree entry; it only pulls the plain
+//! values out of the teloxide update and hands them to [`handle_callback`],
+//! which holds the button logic and is driven directly by tests.

-use super::{CHAT_STORE, CONFIG, TASK_QUEUE};
+use crate::ctx::AppContext;
 use crate::db::unix_now;
 use crate::send::{self, Task};
 use teloxide::RequestError;
 use teloxide::prelude::*;
-use teloxide::types::{CallbackQuery, ChatId, MessageId, ParseMode};
+use teloxide::types::{CallbackQuery, CallbackQueryId, MessageId};
+
+/// The `"forward"` button's data.
+const FORWARD: &str = "forward";
+/// Prefix of a template button's data: `"template|<name>"`.
+const TEMPLATE_PREFIX: &str = "template|";

 pub async fn callback_query_handler(bot: Bot, query: CallbackQuery) -> Result<(), RequestError> {
-    let callback_query_id = query.id;
-    let data = query.data.clone();
    let Some(message) = &query.message else {
        return respond(());
    };
-    let chat_id = message.chat().id.0;
-    let prompt_message_id = message.id().0 as i64;
-    let ttl_secs = CONFIG.edit_message_ttl.as_secs() as i64;
-    let chat_data = CHAT_STORE.get(chat_id).await;
+    let Some(data) = query.data.clone() else {
+        return respond(());
+    };
+    let ctx = AppContext::from_statics(&bot);
+    handle_callback(
+        &ctx,
+        query.id.clone(),
+        message.chat().id.0,
+        message.id().0 as i64,
+        &data,
+    )
+    .await;
+    respond(())
+}
+
+/// Handles one button press on the edit-before-forward prompt.
+async fn handle_callback(
+    ctx: &AppContext<'_>,
+    callback_query_id: CallbackQueryId,
+    chat_id: i64,
+    prompt_message_id: i64,
+    data: &str,
+) {
+    let ttl_secs = ctx.config.edit_message_ttl.as_secs() as i64;
+    let chat_data = ctx.chat_store.get(chat_id).await;
    let edit = chat_data.edit_message.get(&prompt_message_id).cloned();
    let Some(edit) = edit else {
-        log::debug!(
-            "callback from {}: no edit record for prompt {prompt_message_id}",
-            chat_id
-        );
-        bot.answer_callback_query(callback_query_id)
-            .text("Expired")
-            .await?;
-        return respond(());
+        log::debug!("callback from {chat_id}: no edit record for prompt {prompt_message_id}");
+        let _ = ctx
+            .sender
+            .answer_callback_query(callback_query_id, Some("Expired".to_string()))
+            .await;
+        return;
    };
    // Lazy expiry: a stale record (past the TTL, not yet swept) is dropped.
    if edit.created_at + ttl_secs <= unix_now() {
-        CHAT_STORE
+        ctx.chat_store
            .update(chat_id, |data| {
                data.edit_message.remove(&prompt_message_id);
            })
            .await;
-        bot.answer_callback_query(callback_query_id)
-            .text("Expired")
-            .await?;
-        return respond(());
+        let _ = ctx
+            .sender
+            .answer_callback_query(callback_query_id, Some("Expired".to_string()))
+            .await;
+        return;
    }

-    let Some(data) = data else {
-        return respond(());
-    };
-    log::info!(
-        "callback from {} on prompt {prompt_message_id}: {data}",
-        chat_id
-    );
-    if data == "forward" {
-        let ctx = crate::ctx::AppContext::from_statics(&bot);
+    log::info!("callback from {chat_id} on prompt {prompt_message_id}: {data}");
+    if data == FORWARD {
        match chat_data.forward_channel_id {
            Some(channel_id) => {
                let forward_task = Task::ForwardMessages {
@@ -60,62 +80,72 @@ pub async fn callback_query_handler(bot: Bot, query: CallbackQuery) -> Result<()
                    notify_chat_id: Some(chat_id),
                    notify_message_id: Some(prompt_message_id),
                };
-                match send::forward_messages(&ctx, &forward_task).await {
+                let (answer, settled) = match send::forward_messages(ctx, &forward_task).await {
                    Ok(()) => {
                        log::info!(
                            "forwarded {} message(s) to channel {channel_id}",
                            edit.forward_message_ids.len()
                        );
-                        bot.answer_callback_query(callback_query_id)
-                            .text("✅ Forwarded")
-                            .await?;
-                        let _ = bot
-                            .delete_message(ChatId(chat_id), MessageId(prompt_message_id as i32))
-                            .await;
-                        CHAT_STORE
-                            .update(chat_id, |data| {
-                                data.edit_message.remove(&prompt_message_id);
-                            })
-                            .await;
+                        ("✅ Forwarded".to_string(), true)
                    }
                    Err(send::SendError::Retryable {
                        delay_seconds,
                        task,
                    }) => {
                        log::info!("forward queued for retry in {delay_seconds:.1}s");
-                        send::enqueue_retry(&TASK_QUEUE, *task, delay_seconds).await;
-                        bot.answer_callback_query(callback_query_id)
-                            .text("Forward queued for retry.")
-                            .await?;
+                        send::enqueue_retry(ctx.task_queue, *task, delay_seconds).await;
+                        ("Forward queued for retry.".to_string(), false)
                    }
                    Err(send::SendError::Permanent { message, .. }) => {
                        log::error!("forward failed permanently: {message}");
-                        bot.answer_callback_query(callback_query_id)
-                            .text(format!("Forward failed: {message}"))
-                            .await?;
+                        (format!("Forward failed: {message}"), false)
                    }
+                };
+                if settled {
+                    // The prompt is done: drop it and its record.
+                    let _ = ctx
+                        .sender
+                        .delete_message(ChatId(chat_id), MessageId(prompt_message_id as i32))
+                        .await;
+                    ctx.chat_store
+                        .update(chat_id, |data| {
+                            data.edit_message.remove(&prompt_message_id);
+                        })
+                        .await;
                }
+                let _ = ctx
+                    .sender
+                    .answer_callback_query(callback_query_id, Some(answer))
+                    .await;
            }
            None => {
                log::debug!("forward callback without a forward channel set");
-                bot.answer_callback_query(callback_query_id)
-                    .text("No forward channel set.")
-                    .await?;
+                let _ = ctx
+                    .sender
+                    .answer_callback_query(
+                        callback_query_id,
+                        Some("No forward channel set.".to_string()),
+                    )
+                    .await;
            }
        }
-        return respond(());
+        return;
    }
-    if let Some(name) = data.strip_prefix("template|") {
+
+    if let Some(name) = data.strip_prefix(TEMPLATE_PREFIX) {
        if let Some(template_html) = chat_data.template.get(name).cloned()
            && let Some(first_forward_id) = edit.forward_message_ids.first().copied()
        {
            // Raw template including the [] placeholder (Python parity).
-            let _ = bot
-                .edit_message_caption(ChatId(chat_id), MessageId(first_forward_id as i32))
-                .caption(template_html)
-                .parse_mode(ParseMode::Html)
+            let _ = ctx
+                .sender
+                .edit_message_caption(
+                    ChatId(chat_id),
+                    MessageId(first_forward_id as i32),
+                    template_html,
+                )
                .await;
-            CHAT_STORE
+            ctx.chat_store
                .update(chat_id, |data| {
                    if let Some(entry) = data.edit_message.get_mut(&prompt_message_id) {
                        entry.template = name.to_string();
@@ -124,7 +154,168 @@ pub async fn callback_query_handler(bot: Bot, query: CallbackQuery) -> Result<()
                .await;
            log::info!("template '{name}' applied to prompt {prompt_message_id}");
        }
-        bot.answer_callback_query(callback_query_id).await?;
+        let _ = ctx
+            .sender
+            .answer_callback_query(callback_query_id, None)
+            .await;
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::ctx::test_support::TestStores;
+    use crate::media_sender::test_support::{MockSender, Outcome};
+    use crate::state::EditMessage;
+    use teloxide::ApiError;
+
+    /// The edit-before-forward prompt's message id in these tests.
+    const PROMPT_ID: i64 = 7;
+    /// The message the prompt refers to (the one whose caption is swapped).
+    const FORWARDED_ID: i64 = 9;
+
+    fn api_error() -> RequestError {
+        RequestError::Api(ApiError::Unknown("Bad Request: chat not found".into()))
+    }
+
+    fn callback_id() -> CallbackQueryId {
+        CallbackQueryId("cb-1".to_string())
+    }
+
+    /// Seeds a live prompt record plus a forward channel and a template;
+    /// `created_at` backdates the record for the expiry cases.
+    async fn seed_prompt(ctx: &AppContext<'_>, created_at: i64) {
+        ctx.chat_store
+            .update(1, |data| {
+                data.forward_channel_id = Some(2);
+                data.template
+                    .insert("tpl".to_string(), "<b>[]</b>".to_string());
+                data.edit_message.insert(
+                    PROMPT_ID,
+                    EditMessage {
+                        url: "https://x.com/u/status/1".into(),
+                        chat_id: 1,
+                        forward_message_ids: vec![FORWARDED_ID],
+                        template: String::new(),
+                        created_at,
+                    },
+                );
+            })
+            .await;
+    }
+
+    #[tokio::test]
+    async fn template_button_swaps_the_caption_and_records_the_choice() {
+        let sender = MockSender::scripted(vec![Outcome::EditOk], api_error);
+        let stores = TestStores::new();
+        let ctx = stores.ctx(&sender);
+        seed_prompt(&ctx, crate::db::unix_now()).await;
+
+        handle_callback(&ctx, callback_id(), 1, PROMPT_ID, "template|tpl").await;
+
+        assert_eq!(
+            sender.calls(),
+            vec!["edit_message_caption", "answer_callback_query"]
+        );
+        // The raw template, including the [] the user edits into.
+        assert_eq!(sender.captions(), vec!["<b>[]</b>"]);
+        assert_eq!(sender.answers(), vec![None]);
+        let data = ctx.chat_store.get(1).await;
+        assert_eq!(data.edit_message[&PROMPT_ID].template, "tpl");
+    }
+
+    #[tokio::test]
+    async fn forward_button_copies_then_clears_the_prompt() {
+        let sender = MockSender::scripted(vec![Outcome::CopyOk], api_error);
+        let stores = TestStores::new();
+        let ctx = stores.ctx(&sender);
+        seed_prompt(&ctx, crate::db::unix_now()).await;
+
+        handle_callback(&ctx, callback_id(), 1, PROMPT_ID, "forward").await;
+
+        assert_eq!(
+            sender.calls(),
+            vec!["copy_messages", "delete_message", "answer_callback_query"]
+        );
+        assert_eq!(sender.answers(), vec![Some("✅ Forwarded".to_string())]);
+        assert!(
+            ctx.chat_store.get(1).await.edit_message.is_empty(),
+            "a settled prompt must drop its record"
+        );
+    }
+
+    #[tokio::test]
+    async fn forward_without_a_channel_is_reported() {
+        let sender = MockSender::scripted(vec![], api_error);
+        let stores = TestStores::new();
+        let ctx = stores.ctx(&sender);
+        seed_prompt(&ctx, crate::db::unix_now()).await;
+        ctx.chat_store
+            .update(1, |data| data.forward_channel_id = None)
+            .await;
+
+        handle_callback(&ctx, callback_id(), 1, PROMPT_ID, "forward").await;
+
+        assert_eq!(sender.calls(), vec!["answer_callback_query"]);
+        assert_eq!(
+            sender.answers(),
+            vec![Some("No forward channel set.".to_string())]
+        );
+    }
+
+    #[tokio::test]
+    async fn retryable_forward_is_queued_and_keeps_the_prompt() {
+        use teloxide::types::Seconds;
+        let sender = MockSender::scripted(vec![Outcome::CopyErr], || {
+            RequestError::RetryAfter(Seconds::from_seconds(7))
+        });
+        let stores = TestStores::new();
+        let ctx = stores.ctx(&sender);
+        seed_prompt(&ctx, crate::db::unix_now()).await;
+
+        handle_callback(&ctx, callback_id(), 1, PROMPT_ID, "forward").await;
+
+        assert_eq!(
+            sender.calls(),
+            vec!["copy_messages", "answer_callback_query"]
+        );
+        assert_eq!(
+            sender.answers(),
+            vec![Some("Forward queued for retry.".to_string())]
+        );
+        assert_eq!(stores.queued_tasks().await, 1);
+        // The prompt is not settled: the queued retry still needs the record.
+        assert!(
+            ctx.chat_store
+                .get(1)
+                .await
+                .edit_message
+                .contains_key(&PROMPT_ID)
+        );
+    }
+
+    #[tokio::test]
+    async fn unknown_and_expired_prompts_answer_expired() {
+        let sender = MockSender::scripted(vec![], api_error);
+        let stores = TestStores::new();
+        let ctx = stores.ctx(&sender);
+
+        // No record at all.
+        handle_callback(&ctx, callback_id(), 1, PROMPT_ID, "forward").await;
+        assert_eq!(sender.answers(), vec![Some("Expired".to_string())]);
+
+        // A record past its TTL (nothing swept it yet) is dropped on use.
+        let stale = crate::db::unix_now() - ctx.config.edit_message_ttl.as_secs() as i64 - 1;
+        seed_prompt(&ctx, stale).await;
+        handle_callback(&ctx, callback_id(), 1, PROMPT_ID, "forward").await;
+        assert_eq!(
+            sender.answers(),
+            vec![Some("Expired".to_string()), Some("Expired".to_string())]
+        );
+        assert!(
+            ctx.chat_store.get(1).await.edit_message.is_empty(),
+            "the expired record must be dropped"
+        );
+        assert_eq!(sender.calls(), vec!["answer_callback_query"; 2]);
    }
-    respond(())
 }
@@ -18,6 +18,7 @@ pub use inline::inline_query_handler;
 pub use statics::{CHAT_STORE, CONFIG, LINK_CACHE, TASK_QUEUE};
 pub use urls::{start_url_workers, stop_url_workers};

+use crate::ctx::AppContext;
 use crate::media_sender::MediaSender;
 use commands::{Command, execute_command};
 use teloxide::RequestError;
@@ -27,16 +28,13 @@ use teloxide::utils::command::BotCommands;
 use urls::{URL_JOBS, extract_urls};

 /// Reply to a message by id, keeping the reply decoration even if the
-/// original was already deleted.
-pub(crate) async fn reply<T>(
+/// original was already deleted. Returns the reply's message id.
+pub(crate) async fn reply(
    sender: &dyn MediaSender,
    chat_id: i64,
    reply_to: MessageId,
-    text: T,
-) -> Result<Message, RequestError>
-where
-    T: Into<String>,
-{
+    text: impl Into<String>,
+) -> Result<i64, RequestError> {
    sender
        .send_message(ChatId(chat_id), text.into(), Some(reply_to), None)
        .await
@@ -50,13 +48,14 @@ pub(crate) async fn reply_html(
    chat_id: i64,
    reply_to: MessageId,
    text: String,
-) -> Result<Message, RequestError> {
+) -> Result<i64, RequestError> {
    // `<Bot as Requester>::` disambiguates from the MediaSender trait's
    // same-named method (see media_sender.rs).
    <Bot as Requester>::send_message(bot, ChatId(chat_id), text)
        .parse_mode(ParseMode::Html)
        .reply_parameters(ReplyParameters::new(reply_to).allow_sending_without_reply())
        .await
+        .map(|message| message.id.0 as i64)
 }

 /// Log prefix tying the whole lifecycle of one link (fetch → send → cache →
@@ -69,16 +68,16 @@ pub fn log_key(url: &str) -> String {

 /// Edit-before-forward: a reply to the prompt swaps the caption of the first
 /// forwarded message. Returns true when the message was consumed as an edit.
-async fn edit_message_handler(bot: &Bot, message: &Message) -> bool {
-    let Some(reply) = message.reply_to_message() else {
-        return false;
-    };
-    let chat_id = message.chat.id.0;
-    let Some(text) = message.text() else {
-        return false;
-    };
-    let chat_data = CHAT_STORE.get(chat_id).await;
-    let Some(edit) = chat_data.edit_message.get(&(reply.id.0 as i64)) else {
+/// Body of [`message_handler`]'s edit branch, without teloxide update types so
+/// it can be driven by tests.
+async fn edit_message_handler(
+    ctx: &AppContext<'_>,
+    chat_id: i64,
+    reply_to_message_id: i64,
+    text: &str,
+) -> bool {
+    let chat_data = ctx.chat_store.get(chat_id).await;
+    let Some(edit) = chat_data.edit_message.get(&reply_to_message_id) else {
        return false;
    };
    let Some(first_forward_id) = edit.forward_message_ids.first() else {
@@ -98,15 +97,17 @@ async fn edit_message_handler(bot: &Bot, message: &Message) -> bool {
            .map(|template| template.replace("[]", &link))
            .unwrap_or(link)
    };
-    let result = bot
-        .edit_message_caption(ChatId(chat_id), MessageId(*first_forward_id as i32))
-        .caption(new_text)
-        .parse_mode(ParseMode::Html)
-        .await;
-    match result {
-        Ok(_) => log::info!(
-            "edit-before-forward: caption swapped on message {first_forward_id} for prompt {}",
-            reply.id.0
+    match ctx
+        .sender
+        .edit_message_caption(
+            ChatId(chat_id),
+            MessageId(*first_forward_id as i32),
+            new_text,
+        )
+        .await
+    {
+        Ok(()) => log::info!(
+            "edit-before-forward: caption swapped on message {first_forward_id} for prompt {reply_to_message_id}"
        ),
        Err(e) => log::error!("edit_message_caption failed: {e}"),
    }
@@ -133,7 +134,17 @@ pub async fn message_handler(bot: Bot, message: Message) -> Result<(), RequestEr
        message.chat.id
    );
    // URL/edit flows only run in private chats; commands run in any chat.
-    if is_private && edit_message_handler(&bot, &message).await {
+    if is_private
+        && let Some(reply) = message.reply_to_message()
+        && let Some(text) = message.text()
+        && edit_message_handler(
+            &AppContext::from_statics(&bot),
+            message.chat.id.0,
+            reply.id.0 as i64,
+            text,
+        )
+        .await
+    {
        return respond(());
    }
    if let Some(text) = message.text()
@@ -167,3 +178,97 @@ pub async fn message_handler(bot: Bot, message: Message) -> Result<(), RequestEr
    }
    respond(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::ctx::test_support::TestStores;
+    use crate::media_sender::test_support::{MockSender, Outcome};
+    use crate::state::EditMessage;
+    use teloxide::ApiError;
+
+    const PROMPT_ID: i64 = 7;
+    const FORWARDED_ID: i64 = 9;
+
+    fn api_error() -> RequestError {
+        RequestError::Api(ApiError::Unknown("Bad Request: message not found".into()))
+    }
+
+    /// Seeds a prompt record; `template` names the chat template used for it
+    /// (empty = none, the caption gets the bare link).
+    async fn seed_prompt(ctx: &AppContext<'_>, template: &str) {
+        ctx.chat_store
+            .update(1, |data| {
+                data.template
+                    .insert("tpl".to_string(), "<b>[]</b>".to_string());
+                data.edit_message.insert(
+                    PROMPT_ID,
+                    EditMessage {
+                        url: "https://x.com/u/status/1".into(),
+                        chat_id: 1,
+                        forward_message_ids: vec![FORWARDED_ID],
+                        template: template.to_string(),
+                        created_at: crate::db::unix_now(),
+                    },
+                );
+            })
+            .await;
+    }
+
+    #[tokio::test]
+    async fn reply_to_a_prompt_swaps_the_caption_through_its_template() {
+        let sender = MockSender::scripted(vec![Outcome::EditOk], api_error);
+        let stores = TestStores::new();
+        let ctx = stores.ctx(&sender);
+        seed_prompt(&ctx, "tpl").await;
+
+        let consumed = edit_message_handler(&ctx, 1, PROMPT_ID, "new caption").await;
+
+        assert!(consumed, "a reply to the prompt must be consumed");
+        assert_eq!(
+            sender.captions(),
+            vec!["<b><a href=\"https://x.com/u/status/1\">new caption</a></b>"]
+        );
+    }
+
+    #[tokio::test]
+    async fn reply_text_and_url_are_escaped_into_the_caption() {
+        let sender = MockSender::scripted(vec![Outcome::EditOk], api_error);
+        let stores = TestStores::new();
+        let ctx = stores.ctx(&sender);
+        seed_prompt(&ctx, "").await;
+
+        edit_message_handler(&ctx, 1, PROMPT_ID, "<script>alert(1)</script>").await;
+
+        // No raw markup from user text may reach the HTML caption.
+        assert_eq!(
+            sender.captions(),
+            vec!["<a href=\"https://x.com/u/status/1\">&lt;script&gt;alert(1)&lt;/script&gt;</a>"]
+        );
+    }
+
+    #[tokio::test]
+    async fn a_failed_caption_swap_still_consumes_the_reply() {
+        let sender = MockSender::scripted(vec![Outcome::EditErr], api_error);
+        let stores = TestStores::new();
+        let ctx = stores.ctx(&sender);
+        seed_prompt(&ctx, "tpl").await;
+
+        // The edit failed (message deleted etc.); the reply must still be
+        // swallowed instead of being treated as a link to fetch.
+        assert!(edit_message_handler(&ctx, 1, PROMPT_ID, "new caption").await);
+        assert_eq!(sender.calls(), vec!["edit_message_caption"]);
+    }
+
+    #[tokio::test]
+    async fn reply_to_an_unrelated_message_is_not_consumed() {
+        let sender = MockSender::scripted(vec![], api_error);
+        let stores = TestStores::new();
+        let ctx = stores.ctx(&sender);
+
+        // No prompt record for that message id → the reply runs the normal
+        // (URL/command) path instead.
+        assert!(!edit_message_handler(&ctx, 1, PROMPT_ID, "hello").await);
+        assert!(sender.calls().is_empty());
+    }
+}