Remove JIRA in-progress transition on PR open (#182)

This has been troublesome in a couple different ways:

- Sometimes PRs get opened accidentally against the wrong release
  branch, which results in lots of JIRA comments that are wrong
  and confusing.
- Sometimes resolving a JIRA requires multiple PRs and having lots
  of these "submitted for review" comments quickly becomes noise.
- Also check transition before changing it to avoid extra jira churn.

Author: matthauck

src/jira/api.rs

@@ -12,6 +12,7 @@ use jira::models::*;
 use version;
 
 pub trait Session: Send + Sync {
+    fn get_issue(&self, key: &str) -> Result<Issue>;
     fn get_transitions(&self, key: &str) -> Result<Vec<Transition>>;
 
     fn transition_issue(&self, key: &str, transition: &TransitionRequest) -> Result<()>;
@@ -93,6 +94,12 @@ impl JiraSession {
 }
 
 impl Session for JiraSession {
+    fn get_issue(&self, key: &str) -> Result<Issue> {
+        self.client
+            .get::<Issue>(&format!("/issue/{}", key))
+            .map_err(|e| Error::from(format!("Error creating getting issue [{}]: {}", key, e)))
+    }
+
     fn get_transitions(&self, key: &str) -> Result<Vec<Transition>> {
         #[derive(Deserialize)]
         struct TransitionsResp {

src/jira/models.rs

@@ -1,4 +1,14 @@
 
+#[derive(Deserialize, Serialize, Clone, Debug)]
+pub struct Issue {
+    pub key: String,
+    pub status: Option<Status>,
+}
+
+#[derive(Deserialize, Serialize, Clone, Debug)]
+pub struct Status {
+    pub name: String,
+}
 
 #[derive(Deserialize, Serialize, Clone, Debug)]
 pub struct Comment {

src/jira/workflow.rs

@@ -5,7 +5,7 @@ use version;
 
 use config::JiraConfig;
 use errors::*;
-use github::{Commit, CommitLike, PullRequest, PushCommit};
+use github::{Commit, CommitLike, PushCommit};
 use jira;
 use jira::Transition;
 
@@ -67,47 +67,48 @@ fn get_jira_project(jira_key: &str) -> &str {
     }
 }
 
+fn needs_transition(state: &Option<jira::Status>, target: &Vec<String>) -> bool {
+    if let &Some(ref state) = state {
+        return !target.contains(&state.name);
+    } else {
+        return true;
+    }
+}
+
 pub fn submit_for_review(
-    pr: &PullRequest,
     commits: &Vec<Commit>,
     projects: &Vec<String>,
     jira: &jira::api::Session,
     config: &JiraConfig,
 ) {
+    let review_states = config.review_states();
+    let progress_states = config.progress_states();
+
     for key in get_fixed_jira_keys(commits, projects) {
-        // add comment
-        if let Err(e) = jira.comment_issue(
-            &key,
-            &format!("Review submitted for branch {}: {}", pr.base.ref_name, pr.html_url),
-        )
-        {
-            error!("Error commenting on key [{}]: {}", key, e);
-            continue; // give up on transitioning if we can't comment.
+        let issue_state = try_get_issue_state(&key, jira);
+
+        if !needs_transition(&issue_state, &review_states) {
+            continue;
         }
 
         // try to transition to in-progress
-        try_transition(&key, &config.progress_states(), jira);
+        if needs_transition(&issue_state, &progress_states) {
+            try_transition(&key, &progress_states, jira);
+        }
+
         // try transition to pending-review
-        try_transition(&key, &config.review_states(), jira);
+        try_transition(&key, &review_states, jira);
     }
 
     for key in get_referenced_jira_keys(commits, projects) {
-        // add comment
-        if let Err(e) = jira.comment_issue(
-            &key,
-            &format!(
-                "Referenced by review submitted for branch {}: {}",
-                pr.base.ref_name,
-                pr.html_url
-            ),
-        )
-        {
-            error!("Error commenting on key [{}]: {}", key, e);
-            continue; // give up on transitioning if we can't comment.
+        let issue_state = try_get_issue_state(&key, jira);
+
+        if !needs_transition(&issue_state, &progress_states) {
+            continue;
         }
 
         // try to transition to in-progress
-        try_transition(&key, &config.progress_states(), jira);
+        try_transition(&key, &progress_states, jira);
     }
 }
 
@@ -134,14 +135,19 @@ pub fn resolve_issue(
 
         let fix_msg = format!("Merged into branch {}: {}{}", branch, desc, version_desc);
         let ref_msg = format!("Referenced by commit merged into branch {}: {}{}", branch, desc, version_desc);
+        let resolved_states = config.resolved_states();
 
         for key in get_fixed_jira_keys(&vec![commit], projects) {
             if let Err(e) = jira.comment_issue(&key, &fix_msg) {
                 error!("Error commenting on key [{}]: {}", key, e);
             }
 
-            let to = config.resolved_states();
-            match find_transition(&key, &to, jira) {
+            let issue_state = try_get_issue_state(&key, jira);
+            if !needs_transition(&issue_state, &resolved_states) {
+                continue;
+            }
+
+            match find_transition(&key, &resolved_states, jira) {
                 Ok(Some(transition)) => {
                     let mut req = transition.new_request();
 
@@ -168,12 +174,12 @@ pub fn resolve_issue(
                     }
 
                     if let Err(e) = jira.transition_issue(&key, &req) {
-                        error!("Error transitioning JIRA issue [{}] to one of [{:?}]: {}", key, to, e);
+                        error!("Error transitioning JIRA issue [{}] to one of [{:?}]: {}", key, resolved_states, e);
                     } else {
-                        info!("Transitioned [{}] to one of [{:?}]", key, to);
+                        info!("Transitioned [{}] to one of [{:?}]", key, resolved_states);
                     }
                 }
-                Ok(None) => info!("JIRA [{}] cannot be transitioned to  any of [{:?}]", key, to),
+                Ok(None) => info!("JIRA [{}] cannot be transitioned to  any of [{:?}]", key, resolved_states),
                 Err(e) => error!("{}", e),
             };
         }
@@ -308,6 +314,16 @@ fn find_relevant_versions(
         .collect::<Vec<_>>()
 }
 
+fn try_get_issue_state(key: &str, jira: &jira::api::Session) -> Option<jira::Status> {
+    match jira.get_issue(key) {
+        Ok(issue) => issue.status,
+        Err(e) => {
+            error!("Error getting JIRA [{}] {}", key, e);
+            None
+        },
+    }
+}
+
 fn try_transition(key: &str, to: &Vec<String>, jira: &jira::api::Session) {
     match find_transition(&key, to, jira) {
         Ok(Some(transition)) => {

src/server/github_handler.rs

@@ -57,6 +57,7 @@ pub struct GithubEventHandler {
 const MAX_CONCURRENT_MERGES: usize = 20;
 const MAX_CONCURRENT_VERSIONS: usize = 20;
 const MAX_CONCURRENT_FORCE_PUSH: usize = 20;
+const MAX_COMMITS_FOR_JIRA_CONSIDERATION: usize = 20;
 
 impl GithubHandlerState {
     pub fn new(
@@ -375,15 +376,15 @@ impl GithubEventHandler {
             if let Some(ref verb) = verb {
                 let commits = self.pull_request_commits(&pull_request);
 
+                let attachments = vec![
+                    SlackAttachmentBuilder::new("")
+                        .title(format!("Pull Request #{}: \"{}\"", pull_request.number, pull_request.title.as_str()))
+                        .title_link(pull_request.html_url.as_str())
+                        .build(),
+                ];
+
                 if !pull_request.is_wip() {
                     let msg = format!("Pull Request {}", verb);
-                    let attachments =
-                        vec![SlackAttachmentBuilder::new("")
-                                               .title(format!("Pull Request #{}: \"{}\"",
-                                                              pull_request.number,
-                                                              pull_request.title.as_str()))
-                                               .title_link(pull_request.html_url.as_str())
-                                               .build()];
 
                     if notify_channel_only {
                         self.messenger.send_to_channel(&msg, &attachments, &self.data.repository);
@@ -404,16 +405,26 @@ impl GithubEventHandler {
                 if self.action == "opened" {
                     if let Some(ref jira_config) = self.config.jira {
                         if let Some(ref jira_session) = self.jira_session {
-                            let jira_projects =
-                                self.config.repos().jira_projects(&self.data.repository, &pull_request.base.ref_name);
-
-                            jira::workflow::submit_for_review(
-                                &pull_request,
-                                &commits,
-                                &jira_projects,
-                                jira_session.deref(),
-                                jira_config,
-                            );
+                            if commits.len() > MAX_COMMITS_FOR_JIRA_CONSIDERATION {
+                                let msg = format!(
+                                    "Too many commits on Pull Request #{}. Ignoring JIRAs.",
+                                    pull_request.number
+                                );
+                                self.messenger.send_to_owner(&msg, &attachments, &pull_request.user, &self.data.repository);
+
+                            } else {
+                                let jira_projects = self.config.repos().jira_projects(
+                                    &self.data.repository,
+                                    &pull_request.base.ref_name,
+                                );
+
+                                jira::workflow::submit_for_review(
+                                    &commits,
+                                    &jira_projects,
+                                    jira_session.deref(),
+                                    jira_config,
+                                );
+                            }
                         }
                     }
                 }

tests/github_handler_test.rs

@@ -1374,6 +1374,13 @@ fn test_push_force_notify_ignored() {
     expect_thread.join().unwrap();
 }
 
+fn new_issue(key: &str) -> jira::Issue {
+    jira::Issue {
+        key: key.into(),
+        status: None,
+    }
+}
+
 fn new_transition(id: &str, name: &str) -> jira::Transition {
     jira::Transition {
         id: id.into(),
@@ -1407,6 +1414,17 @@ fn some_jira_commits() -> Vec<Commit> {
     ]
 }
 
+fn many_jira_commits() -> Vec<Commit> {
+    let commit = Commit {
+            sha: "ffeedd00110011".into(),
+            html_url: "http://commit/ffeedd00110011".into(),
+            author: Some(User::new("bob-author")),
+            commit: CommitDetails { message: "Fix [SER-1] Add the feature\n\nThe body ([OTHER-123])".into() },
+        };
+
+    return (0..21).collect::<Vec<u32>>().into_iter().map(|_| commit.clone()).collect();
+}
+
 fn some_jira_push_commits() -> Vec<PushCommit> {
     vec![
         PushCommit {
@@ -1417,6 +1435,7 @@ fn some_jira_push_commits() -> Vec<PushCommit> {
         },
     ]
 }
+
 #[test]
 fn test_jira_pull_request_opened() {
     let mut test = new_test_with_jira();
@@ -1449,11 +1468,7 @@ fn test_jira_pull_request_opened() {
     ]);
 
     if let Some(ref jira) = test.jira {
-        jira.mock_comment_issue(
-            "SER-1",
-            "Review submitted for branch master: http://the-pr",
-            Ok(()),
-        );
+        jira.mock_get_issue("SER-1", Ok(new_issue("SER-1")));
 
         jira.mock_get_transitions("SER-1", Ok(vec![new_transition("001", "the-progress")]));
         jira.mock_transition_issue("SER-1", &new_transition_req("001"), Ok(()));
@@ -1466,6 +1481,52 @@ fn test_jira_pull_request_opened() {
     assert_eq!((StatusCode::Ok, "pr".into()), resp);
 }
 
+#[test]
+fn test_jira_pull_request_opened_too_many_commits() {
+    let mut test = new_test_with_jira();
+    test.handler.event = "pull_request".into();
+    test.handler.action = "opened".into();
+    test.handler.data.pull_request = some_pr();
+    test.handler.data.sender = User::new("the-pr-owner");
+
+    test.github.mock_get_pull_request_commits(
+        "some-user",
+        "some-repo",
+        32,
+        Ok(many_jira_commits()),
+    );
+
+    let attach = vec![
+        SlackAttachmentBuilder::new("")
+            .title("Pull Request #32: \"The PR\"")
+            .title_link("http://the-pr")
+            .build(),
+    ];
+
+    test.slack.expect(vec![
+        slack::req(
+            "the-reviews-channel",
+            &format!("Pull Request opened by the.pr.owner {}", REPO_MSG),
+            attach.clone()
+        ),
+        slack::req(
+            "the-reviews-channel",
+            &format!("Too many commits on Pull Request #32. Ignoring JIRAs. {}", REPO_MSG),
+            attach.clone()
+        ),
+        slack::req(
+            "@the.pr.owner",
+            &format!("Too many commits on Pull Request #32. Ignoring JIRAs."),
+            attach.clone()
+        ),
+    ]);
+
+    // do not set jira expectations
+
+    let resp = test.handler.handle_event().unwrap();
+    assert_eq!((StatusCode::Ok, "pr".into()), resp);
+}
+
 #[test]
 fn test_jira_push_master() {
     let mut test = new_test_with_jira();
@@ -1476,6 +1537,7 @@ fn test_jira_push_master() {
     test.handler.data.commits = Some(some_jira_push_commits());
 
     if let Some(ref jira) = test.jira {
+        jira.mock_get_issue("SER-1", Ok(new_issue("SER-1")));
         jira.mock_comment_issue("SER-1", "Merged into branch master: [ffeedd0|http://commit/ffeedd00110011]\n{quote}Fix [SER-1] Add the feature{quote}", Ok(()));
 
         jira.mock_get_transitions("SER-1", Ok(vec![new_transition("003", "the-resolved")]));
@@ -1496,6 +1558,7 @@ fn test_jira_push_develop() {
     test.handler.data.commits = Some(some_jira_push_commits());
 
     if let Some(ref jira) = test.jira {
+        jira.mock_get_issue("SER-1", Ok(new_issue("SER-1")));
         jira.mock_comment_issue("SER-1", "Merged into branch develop: [ffeedd0|http://commit/ffeedd00110011]\n{quote}Fix [SER-1] Add the feature{quote}", Ok(()));
 
         jira.mock_get_transitions("SER-1", Ok(vec![new_transition("003", "the-resolved")]));
@@ -1516,6 +1579,7 @@ fn test_jira_push_release() {
     test.handler.data.commits = Some(some_jira_push_commits());
 
     if let Some(ref jira) = test.jira {
+        jira.mock_get_issue("SER-1", Ok(new_issue("SER-1")));
         jira.mock_comment_issue("SER-1", "Merged into branch release/55: [ffeedd0|http://commit/ffeedd00110011]\n{quote}Fix [SER-1] Add the feature{quote}", Ok(()));
 
         jira.mock_get_transitions("SER-1", Ok(vec![new_transition("003", "the-resolved")]));