Ping added mentions on modified comments/issues messages

src/handlers/notification.rs

@@ -21,20 +21,11 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> {
     };
 
     if let Event::Issue(e) = event {
-        if e.action != github::IssuesAction::Opened {
-            // skip events other than opening the issue to avoid retriggering commands in the
-            // issue body
-            return Ok(());
-        }
-    }
-
-    if let Event::IssueComment(e) = event {
-        if e.action != github::IssueCommentAction::Created {
-            // skip events other than creating a comment to avoid
-            // renotifying
-            //
-            // FIXME: implement smart tracking to allow rerunning only if
-            // the notification is "new" (i.e. edit adds a ping)
+        if !matches!(
+            e.action,
+            github::IssuesAction::Opened | github::IssuesAction::Edited
+        ) {
+            // no change in issue's body for these events, so skip
             return Ok(());
         }
     }
@@ -60,7 +51,17 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> {
         }
     }
 
+    // Get the list of users already notified by a previous version of this
+    // comment, so they don't get notified again
     let mut users_notified = HashSet::new();
+    if let Some(from) = event.comment_from() {
+        for login in parser::get_mentions(from).into_iter() {
+            if let Some((Ok(users), _)) = id_from_user(ctx, login).await? {
+                users_notified.extend(users.into_iter().map(|user| user.id.unwrap()));
+            }
+        }
+    };
+
     // We've implicitly notified the user that is submitting the notification:
     // they already know that they left this comment.
     //
@@ -77,79 +78,9 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> {
     }
     log::trace!("Captured usernames in comment: {:?}", caps);
     for login in caps {
-        let (users, team_name) = if login.contains('/') {
-            // This is a team ping. For now, just add it to everyone's agenda on
-            // that team, but also mark it as such (i.e., a team ping) for
-            // potentially different prioritization and so forth.
-            //
-            // In order to properly handle this down the road, we will want to
-            // distinguish between "everyone must pay attention" and "someone
-            // needs to take a look."
-            //
-            // We may also want to be able to categorize into these buckets
-            // *after* the ping occurs and is initially processed.
-
-            let mut iter = login.split('/');
-            let _rust_lang = iter.next().unwrap();
-            let team = iter.next().unwrap();
-            let team = match github::get_team(&ctx.github, team).await {
-                Ok(Some(team)) => team,
-                Ok(None) => {
-                    log::error!("team ping ({}) failed to resolve to a known team", login);
-                    continue;
-                }
-                Err(err) => {
-                    log::error!(
-                        "team ping ({}) failed to resolve to a known team: {:?}",
-                        login,
-                        err
-                    );
-                    continue;
-                }
-            };
-
-            (
-                team.members
-                    .into_iter()
-                    .map(|member| {
-                        let id = i64::try_from(member.github_id).with_context(|| {
-                            format!("user id {} out of bounds", member.github_id)
-                        })?;
-                        Ok(github::User {
-                            id: Some(id),
-                            login: member.github,
-                        })
-                    })
-                    .collect::<anyhow::Result<Vec<github::User>>>(),
-                Some(team.name),
-            )
-        } else {
-            let user = github::User {
-                login: login.to_owned(),
-                id: None,
-            };
-            let id = user
-                .get_id(&ctx.github)
-                .await
-                .with_context(|| format!("failed to get user {} ID", user.login))?;
-            let id = match id {
-                Some(id) => id,
-                // If the user was not in the team(s) then just don't record it.
-                None => {
-                    log::trace!("Skipping {} because no id found", user.login);
-                    continue;
-                }
-            };
-            let id = i64::try_from(id).with_context(|| format!("user id {} out of bounds", id));
-            (
-                id.map(|id| {
-                    vec![github::User {
-                        login: user.login.clone(),
-                        id: Some(id),
-                    }]
-                }),
-                None,
-            )
+        let (users, team_name) = match id_from_user(ctx, login).await? {
+            Some((users, team_name)) => (users, team_name),
+            None => continue,
         };
 
         let users = match users {
@@ -194,3 +125,82 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> {
 
     Ok(())
 }
+
+async fn id_from_user(
+    ctx: &Context,
+    login: &str,
+) -> anyhow::Result<Option<(anyhow::Result<Vec<github::User>>, Option<String>)>> {
+    if login.contains('/') {
+        // This is a team ping. For now, just add it to everyone's agenda on
+        // that team, but also mark it as such (i.e., a team ping) for
+        // potentially different prioritization and so forth.
+        //
+        // In order to properly handle this down the road, we will want to
+        // distinguish between "everyone must pay attention" and "someone
+        // needs to take a look."
+        //
+        // We may also want to be able to categorize into these buckets
+        // *after* the ping occurs and is initially processed.
+
+        let mut iter = login.split('/');
+        let _rust_lang = iter.next().unwrap();
+        let team = iter.next().unwrap();
+        let team = match github::get_team(&ctx.github, team).await {
+            Ok(Some(team)) => team,
+            Ok(None) => {
+                log::error!("team ping ({}) failed to resolve to a known team", login);
+                return Ok(None);
+            }
+            Err(err) => {
+                log::error!(
+                    "team ping ({}) failed to resolve to a known team: {:?}",
+                    login,
+                    err
+                );
+                return Ok(None);
+            }
+        };
+
+        Ok(Some((
+            team.members
+                .into_iter()
+                .map(|member| {
+                    let id = i64::try_from(member.github_id)
+                        .with_context(|| format!("user id {} out of bounds", member.github_id))?;
+                    Ok(github::User {
+                        id: Some(id),
+                        login: member.github,
+                    })
+                })
+                .collect::<anyhow::Result<Vec<github::User>>>(),
+            Some(team.name),
+        )))
+    } else {
+        let user = github::User {
+            login: login.to_owned(),
+            id: None,
+        };
+        let id = user
+            .get_id(&ctx.github)
+            .await
+            .with_context(|| format!("failed to get user {} ID", user.login))?;
+        let id = match id {
+            Some(id) => id,
+            // If the user was not in the team(s) then just don't record it.
+            None => {
+                log::trace!("Skipping {} because no id found", user.login);
+                return Ok(None);
+            }
+        };
+        let id = i64::try_from(id).with_context(|| format!("user id {} out of bounds", id));
+        Ok(Some((
+            id.map(|id| {
+                vec![github::User {
+                    login: user.login.clone(),
+                    id: Some(id),
+                }]
+            }),
+            None,
+        )))
+    }
+}