Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notify Zulip when issue is closed or reopened #1078

Merged
merged 3 commits into from
Aug 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ pub(crate) struct NotifyZulipLabelConfig {
pub(crate) topic: String,
pub(crate) message_on_add: Option<String>,
pub(crate) message_on_remove: Option<String>,
pub(crate) message_on_close: Option<String>,
pub(crate) message_on_reopen: Option<String>,
#[serde(default)]
pub(crate) required_labels: Vec<String>,
}
Expand Down
195 changes: 135 additions & 60 deletions src/handlers/notify_zulip.rs
Original file line number Diff line number Diff line change
@@ -1,96 +1,171 @@
use crate::{
config::NotifyZulipConfig,
github::{IssuesAction, IssuesEvent},
config::{NotifyZulipConfig, NotifyZulipLabelConfig},
github::{Issue, IssuesAction, IssuesEvent, Label},
handlers::Context,
};

pub(super) struct NotifyZulipInput {
notification_type: NotificationType,
/// Label that triggered this notification.
///
/// For example, if an `I-prioritize` issue is closed,
/// this field will be `I-prioritize`.
label: Label,
}

pub(super) enum NotificationType {
Labeled,
Unlabeled,
Closed,
Reopened,
}

pub(super) fn parse_input(
_ctx: &Context,
event: &IssuesEvent,
config: Option<&NotifyZulipConfig>,
) -> Result<Option<NotifyZulipInput>, String> {
if let IssuesAction::Labeled | IssuesAction::Unlabeled = event.action {
let applied_label = &event.label.as_ref().expect("label").name;
if let Some(config) = config.and_then(|c| c.labels.get(applied_label)) {
for label in &config.required_labels {
let pattern = match glob::Pattern::new(label) {
Ok(pattern) => pattern,
Err(err) => {
log::error!("Invalid glob pattern: {}", err);
continue;
}
};
if !event
.issue
.labels()
.iter()
.any(|l| pattern.matches(&l.name))
{
// Issue misses a required label, ignore this event
return Ok(None);
) -> Result<Option<Vec<NotifyZulipInput>>, String> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately there's now two ways to represent a lack of notifications: None and Some(vec![]). However, I don't think there's a way to get rid of that without changing a lot of other code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference between None and Some(vec![]) in this case is that Some(vec![]) will spawn an additional Future that does nothing. I think we can always improve/cleanup later.

let config = match config {
Some(config) => config,
None => return Ok(None),
};

match event.action {
IssuesAction::Labeled | IssuesAction::Unlabeled => {
let applied_label = event.label.as_ref().expect("label").clone();
camelid marked this conversation as resolved.
Show resolved Hide resolved
Ok(config
.labels
.get(&applied_label.name)
.and_then(|label_config| {
parse_label_change_input(event, applied_label, label_config)
})
.map(|input| vec![input]))
}
IssuesAction::Closed | IssuesAction::Reopened => {
Ok(Some(parse_close_reopen_input(event, config)))
}
_ => Ok(None),
}
}

fn parse_label_change_input(
event: &IssuesEvent,
label: Label,
config: &NotifyZulipLabelConfig,
) -> Option<NotifyZulipInput> {
if !has_all_required_labels(&event.issue, config) {
// Issue misses a required label, ignore this event
return None;
}

match event.action {
IssuesAction::Labeled if config.message_on_add.is_some() => Some(NotifyZulipInput {
notification_type: NotificationType::Labeled,
label,
}),
IssuesAction::Unlabeled if config.message_on_remove.is_some() => Some(NotifyZulipInput {
notification_type: NotificationType::Unlabeled,
label,
}),
Comment on lines +62 to +69
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This previously looked like:

            if event.action == IssuesAction::Labeled && config.message_on_add.is_some() {
                return Ok(Some(NotifyZulipInput {
                    notification_type: NotificationType::Labeled,
                }));
            } else if config.message_on_remove.is_some() {
                return Ok(Some(NotifyZulipInput {
                    notification_type: NotificationType::Unlabeled,
                }));
            }

(notice the catch-all else if)

The new version should hopefully be clearer.

_ => None,
}
}

fn parse_close_reopen_input(
event: &IssuesEvent,
global_config: &NotifyZulipConfig,
) -> Vec<NotifyZulipInput> {
event
.issue
.labels
.iter()
.cloned()
.filter_map(|label| {
global_config
.labels
.get(&label.name)
.map(|config| (label, config))
})
.flat_map(|(label, config)| {
if !has_all_required_labels(&event.issue, config) {
// Issue misses a required label, ignore this event
return None;
}

match event.action {
IssuesAction::Closed if config.message_on_close.is_some() => {
Some(NotifyZulipInput {
notification_type: NotificationType::Closed,
label,
})
}
IssuesAction::Reopened if config.message_on_reopen.is_some() => {
Some(NotifyZulipInput {
notification_type: NotificationType::Reopened,
label,
})
}
Comment on lines +82 to 107
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think we could only clone the labels we're actually interested in?

Suggested change
.cloned()
.filter_map(|label| {
global_config
.labels
.get(&label.name)
.map(|config| (label, config))
})
.flat_map(|(label, config)| {
if !has_all_required_labels(&event.issue, config) {
// Issue misses a required label, ignore this event
return None;
}
match event.action {
IssuesAction::Closed if config.message_on_close.is_some() => {
Some(NotifyZulipInput {
notification_type: NotificationType::Closed,
label,
})
}
IssuesAction::Reopened if config.message_on_reopen.is_some() => {
Some(NotifyZulipInput {
notification_type: NotificationType::Reopened,
label,
})
}
.filter_map(|label| {
global_config
.labels
.get(&label.name)
.map(|config| (label, config))
})
.flat_map(|(label, config)| {
if !has_all_required_labels(&event.issue, config) {
// Issue misses a required label, ignore this event
return None;
}
match event.action {
IssuesAction::Closed if config.message_on_close.is_some() => {
Some(NotifyZulipInput {
notification_type: NotificationType::Closed,
label: label.clone(),
})
}
IssuesAction::Reopened if config.message_on_reopen.is_some() => {
Some(NotifyZulipInput {
notification_type: NotificationType::Reopened,
label: label.clone(),
})
}

_ => None,
}
})
.collect()
}

if event.action == IssuesAction::Labeled && config.message_on_add.is_some() {
return Ok(Some(NotifyZulipInput {
notification_type: NotificationType::Labeled,
}));
} else if config.message_on_remove.is_some() {
return Ok(Some(NotifyZulipInput {
notification_type: NotificationType::Unlabeled,
}));
fn has_all_required_labels(issue: &Issue, config: &NotifyZulipLabelConfig) -> bool {
for req_label in &config.required_labels {
let pattern = match glob::Pattern::new(req_label) {
Ok(pattern) => pattern,
Err(err) => {
log::error!("Invalid glob pattern: {}", err);
continue;
}
};
if !issue.labels().iter().any(|l| pattern.matches(&l.name)) {
return false;
}
}
Ok(None)

true
}

pub(super) async fn handle_input<'a>(
ctx: &Context,
config: &NotifyZulipConfig,
event: &IssuesEvent,
input: NotifyZulipInput,
inputs: Vec<NotifyZulipInput>,
) -> anyhow::Result<()> {
Comment on lines 131 to 136
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend hiding whitespace changes when reviewing this function's diff.

let config = config
.labels
.get(&event.label.as_ref().unwrap().name)
.unwrap();

let mut topic = config.topic.clone();
topic = topic.replace("{number}", &event.issue.number.to_string());
topic = topic.replace("{title}", &event.issue.title);
// Truncate to 60 chars (a Zulip limitation)
let mut chars = topic.char_indices().skip(59);
if let (Some((len, _)), Some(_)) = (chars.next(), chars.next()) {
topic.truncate(len);
topic.push('…');
}
for input in inputs {
let config = &config.labels[&input.label.name];

let mut msg = match input.notification_type {
NotificationType::Labeled => config.message_on_add.as_ref().unwrap().clone(),
NotificationType::Unlabeled => config.message_on_remove.as_ref().unwrap().clone(),
};
let mut topic = config.topic.clone();
topic = topic.replace("{number}", &event.issue.number.to_string());
topic = topic.replace("{title}", &event.issue.title);
// Truncate to 60 chars (a Zulip limitation)
let mut chars = topic.char_indices().skip(59);
if let (Some((len, _)), Some(_)) = (chars.next(), chars.next()) {
topic.truncate(len);
topic.push('…');
}

msg = msg.replace("{number}", &event.issue.number.to_string());
msg = msg.replace("{title}", &event.issue.title);
let mut msg = match input.notification_type {
NotificationType::Labeled => config.message_on_add.as_ref().unwrap().clone(),
NotificationType::Unlabeled => config.message_on_remove.as_ref().unwrap().clone(),
NotificationType::Closed => config.message_on_close.as_ref().unwrap().clone(),
NotificationType::Reopened => config.message_on_reopen.as_ref().unwrap().clone(),
};

let zulip_req = crate::zulip::MessageApiRequest {
recipient: crate::zulip::Recipient::Stream {
id: config.zulip_stream,
topic: &topic,
},
content: &msg,
};
zulip_req.send(&ctx.github.raw()).await?;
msg = msg.replace("{number}", &event.issue.number.to_string());
msg = msg.replace("{title}", &event.issue.title);

let zulip_req = crate::zulip::MessageApiRequest {
recipient: crate::zulip::Recipient::Stream {
id: config.zulip_stream,
topic: &topic,
},
content: &msg,
};
zulip_req.send(&ctx.github.raw()).await?;
}

Ok(())
}