feat(merge-versions): display link to target version (#380)

* merge_pending_versions: return id of version object * add link to version page * fix: don't display 'success' message on auth errors * fix unnecessary return * build url on the server-side * Update octobot/src/server/admin.rs Co-authored-by: Matt Hauck <[email protected]>
tanium · Feb 7, 2024 · bc8d332 · bc8d332
1 parent 4f18d1c
commit bc8d332
Show file tree

Hide file tree

Showing 9 changed files with 107 additions and 46 deletions.
diff --git a/lib/src/jira/api.rs b/lib/src/jira/api.rs
@@ -27,7 +27,7 @@ pub trait Session: Send + Sync {
 
     async fn comment_issue(&self, key: &str, comment: &str) -> Result<()>;
 
-    async fn add_version(&self, proj: &str, version: &str) -> Result<()>;
+    async fn add_version(&self, proj: &str, version: &str) -> Result<Version>;
     async fn get_versions(&self, proj: &str) -> Result<Vec<Version>>;
     async fn assign_fix_version(&self, key: &str, version: &str) -> Result<()>;
     async fn reorder_version(&self, version: &Version, position: JiraVersionPosition)
@@ -216,7 +216,7 @@ impl Session for JiraSession {
             .map_err(|e| anyhow!("Error commenting on [{}]: {}", key, e))
     }
 
-    async fn add_version(&self, proj: &str, version: &str) -> Result<()> {
+    async fn add_version(&self, proj: &str, version: &str) -> Result<Version> {
         #[derive(Serialize)]
         struct AddVersionReq {
             name: String,
@@ -227,14 +227,17 @@ impl Session for JiraSession {
             name: version.into(),
             project: proj.into(),
         };
-        self.client.post_void("/version", &req).await.map_err(|e| {
-            anyhow!(
-                "Error adding version {} to project {}: {}",
-                version,
-                proj,
-                e
-            )
-        })
+        self.client
+            .post::<Version, AddVersionReq>("/version", &req)
+            .await
+            .map_err(|e| {
+                anyhow!(
+                    "Error adding version {} to project {}: {}",
+                    version,
+                    proj,
+                    e
+                )
+            })
     }
 
     async fn get_versions(&self, proj: &str) -> Result<Vec<Version>> {

diff --git a/lib/src/jira/models.rs b/lib/src/jira/models.rs
@@ -80,9 +80,13 @@ pub struct Version {
 
 impl Version {
     pub fn new(name: &str) -> Version {
+        Version::with_id(name, "some-id")
+    }
+
+    pub fn with_id(name: &str, id: &str) -> Version {
         Version {
-            uri: "http://something/version/some-id".into(),
-            id: "some-id".into(),
+            uri: format!("http://something/version/{}", id),
+            id: id.into(),
             name: name.into(),
         }
     }

diff --git a/lib/src/jira/workflow.rs b/lib/src/jira/workflow.rs
@@ -306,7 +306,7 @@ pub async fn merge_pending_versions(
     project: &str,
     jira: &dyn jira::api::Session,
     mode: DryRunMode,
-) -> Result<HashMap<String, Vec<version::Version>>> {
+) -> Result<version::MergedVersion> {
     let target_version = match version::Version::parse(version) {
         Some(v) => v,
         None => return Err(anyhow!("Invalid target version: {}", version)),
@@ -328,7 +328,10 @@ pub async fn merge_pending_versions(
         .collect::<HashMap<_, _>>();
 
     if mode == DryRunMode::DryRun {
-        return Ok(all_relevant_versions);
+        return Ok(version::MergedVersion {
+            issues: all_relevant_versions,
+            version_id: None,
+        });
     }
 
     if all_relevant_versions.is_empty() {
@@ -339,18 +342,23 @@ pub async fn merge_pending_versions(
     }
 
     // create the target version for this project
-    if real_versions.iter().any(|v| v.name == version) {
-        info!(
-            "JIRA version {} already exists for project {}",
-            version, project
-        );
-    } else {
-        info!(
-            "Creating new JIRA version {} for project {}",
-            version, project
-        );
-        jira.add_version(project, version).await?;
-    }
+    let id = match real_versions.into_iter().find(|v| v.name == version) {
+        Some(v) => {
+            info!(
+                "JIRA version {} already exists for project {}",
+                version, project
+            );
+            v.id
+        }
+        None => {
+            info!(
+                "Creating new JIRA version {} for project {}",
+                version, project
+            );
+
+            jira.add_version(project, version).await?.id
+        }
+    };
 
     {
         // sort the keys for deterministic results for testing purposes.
@@ -380,7 +388,10 @@ pub async fn merge_pending_versions(
         }
     }
 
-    Ok(all_relevant_versions)
+    Ok(version::MergedVersion {
+        issues: all_relevant_versions,
+        version_id: Some(id),
+    })
 }
 
 fn find_relevant_versions(

diff --git a/lib/src/version.rs b/lib/src/version.rs
@@ -1,4 +1,5 @@
 use std::cmp::{self, Ordering};
+use std::collections::HashMap;
 use std::fmt;
 
 use serde::ser::{Serialize, Serializer};
@@ -8,6 +9,12 @@ pub struct Version {
     parts: Vec<u32>,
 }
 
+#[derive(Clone, Debug, PartialEq)]
+pub struct MergedVersion {
+    pub issues: HashMap<String, Vec<Version>>,
+    pub version_id: Option<String>,
+}
+
 impl Version {
     pub fn parse(version_str: &str) -> Option<Version> {
         let parts = version_str

diff --git a/octobot/src/assets/app.js b/octobot/src/assets/app.js
@@ -474,7 +474,7 @@ app.controller('VersionsController', function($rootScope, $scope, sessionHttp, n
         $scope.req.admin_user = loggedInUser() + resp.data.login_suffix;
       }
       if (resp.data.error) {
-        notificationService.showError(resp.data.error);
+        throw(resp.data.error)
       }
       return resp;
     }).finally(function(e) {
@@ -496,11 +496,13 @@ app.controller('VersionsController', function($rootScope, $scope, sessionHttp, n
 
   function mergeVersionsForReal() {
     let version = $scope.req.version;
+    let project = $scope.req.project;
     mergeVersions(false).then(function(resp) {
-      notificationService.showSuccess('Created new version succesfully');
+      notificationService.showSuccess('Created new version succesfully.');
       $scope.reset();
       $scope.lastResp = resp.data.versions;
       $scope.lastVersion = version;
+      $scope.versionUrl = resp.data.version_url;
 
     }).catch(function(e) {
       notificationService.showError('Error creating new version: ' + parseError(e));

diff --git a/octobot/src/assets/versions.html b/octobot/src/assets/versions.html
@@ -48,7 +48,7 @@ <h4><a ng-href="{{jiraLink(key)}}" target="_blank">{{key}}</a></h4>
   <h3>Success!</h3>
 
   <p>
-    The following JIRAs were found with pending versions were migrated to {{lastVersion}}
+    The following JIRAs were found with pending versions were migrated to <a ng-href="{{versionUrl}}" target="_blank">{{lastVersion}}</a>
   </p>
   <div class="well" ng-repeat="(key, versions) in lastResp">
     <h4><a ng-href="{{jiraLink(key)}}" target="_blank">{{key}}</a></h4>

diff --git a/octobot/src/server/admin.rs b/octobot/src/server/admin.rs
@@ -252,6 +252,7 @@ struct MergeVersionsResp {
     jira_base: String,
     login_suffix: Option<String>,
     versions: HashMap<String, Vec<version::Version>>,
+    version_url: Option<String>,
     error: Option<String>,
 }
 
@@ -261,6 +262,7 @@ impl MergeVersionsResp {
             jira_base: jira_config.base_url(),
             login_suffix: jira_config.login_suffix.clone(),
             versions: HashMap::new(),
+            version_url: None,
             error: None,
         }
     }
@@ -270,6 +272,11 @@ impl MergeVersionsResp {
         self
     }
 
+    fn set_version_url(mut self, version_url: Option<String>) -> Self {
+        self.version_url = version_url;
+        self
+    }
+
     fn set_error(mut self, e: &str) -> Self {
         self.error = Some(e.to_string());
         self
@@ -344,7 +351,18 @@ impl MergeVersions {
             }
         }
 
-        self.make_resp(resp.set_versions(all_relevant_versions))
+        let version_url = match all_relevant_versions.version_id {
+            Some(id) => Some(format!(
+                "{}/projects/{}/versions/{}",
+                resp.jira_base, &merge_req.project, id
+            )),
+            None => None,
+        };
+
+        self.make_resp(
+            resp.set_versions(all_relevant_versions.issues)
+                .set_version_url(version_url),
+        )
     }
 
     fn make_resp(&self, resp: MergeVersionsResp) -> Response<Body> {

diff --git a/octobot/tests/jira_workflow_test.rs b/octobot/tests/jira_workflow_test.rs
@@ -344,7 +344,10 @@ async fn test_merge_pending_versions_for_real() {
     let new_version = "1.2.0.500";
     test.jira.mock_get_versions(
         "SER",
-        Ok(vec![Version::new("1.2.0.100"), Version::new("1.3.0.100")]),
+        Ok(vec![
+            Version::with_id("1.2.0.100", "12345"),
+            Version::with_id("1.3.0.100", "54321"),
+        ]),
     );
     test.jira.mock_find_pending_versions(
         "SER",
@@ -364,7 +367,11 @@ async fn test_merge_pending_versions_for_real() {
         }),
     );
 
-    test.jira.mock_add_version("SER", new_version, Ok(()));
+    test.jira.mock_add_version(
+        "SER",
+        new_version,
+        Ok(Version::with_id(new_version, "89012")),
+    );
 
     test.jira.mock_remove_pending_versions(
         "SER-1",
@@ -382,9 +389,11 @@ async fn test_merge_pending_versions_for_real() {
     test.jira
         .mock_assign_fix_version("SER-4", new_version, Ok(()));
 
-    let res = hashmap! {
+    let res = version::MergedVersion {
+        issues: hashmap! {
         "SER-1".to_string() => vec![version::Version::parse("1.2.0.200").unwrap()],
-        "SER-4".to_string() => vec![version::Version::parse("1.2.0.300").unwrap()],
+        "SER-4".to_string() => vec![version::Version::parse("1.2.0.300").unwrap()] },
+        version_id: Some("89012".to_string()),
     };
 
     assert_eq!(
@@ -407,7 +416,10 @@ async fn test_merge_pending_versions_dry_run() {
     let new_version = "1.2.0.500";
     test.jira.mock_get_versions(
         "SER",
-        Ok(vec![Version::new("1.2.0.100"), Version::new("1.3.0.100")]),
+        Ok(vec![
+            Version::with_id("1.2.0.100", "12345"),
+            Version::with_id("1.3.0.100", "54321"),
+        ]),
     );
     test.jira.mock_find_pending_versions(
         "SER",
@@ -429,9 +441,11 @@ async fn test_merge_pending_versions_dry_run() {
 
     // Don't expect the other state-changing functions to get called
 
-    let res = hashmap! {
+    let res = version::MergedVersion {
+        issues: hashmap! {
         "SER-1".to_string() => vec![version::Version::parse("1.2.0.200").unwrap()],
-        "SER-4".to_string() => vec![version::Version::parse("1.2.0.300").unwrap()],
+        "SER-4".to_string() => vec![version::Version::parse("1.2.0.300").unwrap()] },
+        version_id: None,
     };
 
     assert_eq!(
@@ -455,9 +469,9 @@ async fn test_merge_pending_versions_missed_versions() {
     test.jira.mock_get_versions(
         "SER",
         Ok(vec![
-            Version::new("1.2.0.100"),
-            Version::new("1.2.0.500"),
-            Version::new("1.2.0.600"),
+            Version::with_id("1.2.0.100", "12345"),
+            Version::with_id("1.2.0.500", "54321"),
+            Version::with_id("1.2.0.600", "89012"),
         ]),
     );
     test.jira.mock_find_pending_versions(
@@ -481,8 +495,10 @@ async fn test_merge_pending_versions_missed_versions() {
     test.jira
         .mock_assign_fix_version("SER-1", missed_version, Ok(()));
 
-    let res = hashmap! {
-        "SER-1".to_string() => vec![version::Version::parse("1.2.0.150").unwrap()],
+    let res = version::MergedVersion {
+        issues: hashmap! {
+        "SER-1".to_string() => vec![version::Version::parse("1.2.0.150").unwrap()] },
+        version_id: Some("54321".to_string()),
     };
 
     assert_eq!(

diff --git a/octobot/tests/mocks/mock_jira.rs b/octobot/tests/mocks/mock_jira.rs
@@ -12,7 +12,7 @@ pub struct MockJira {
     get_transitions_calls: Mutex<Vec<MockCall<Vec<Transition>>>>,
     transition_issue_calls: Mutex<Vec<MockCall<()>>>,
     comment_issue_calls: Mutex<Vec<MockCall<()>>>,
-    add_version_calls: Mutex<Vec<MockCall<()>>>,
+    add_version_calls: Mutex<Vec<MockCall<Version>>>,
     get_versions_calls: Mutex<Vec<MockCall<Vec<Version>>>>,
     assign_fix_version_calls: Mutex<Vec<MockCall<()>>>,
     reorder_version_calls: Mutex<Vec<MockCall<()>>>,
@@ -155,7 +155,7 @@ impl Session for MockJira {
         call.ret
     }
 
-    async fn add_version(&self, proj: &str, version: &str) -> Result<()> {
+    async fn add_version(&self, proj: &str, version: &str) -> Result<Version> {
         let mut calls = self.add_version_calls.lock().unwrap();
         assert!(calls.len() > 0, "Unexpected call to add_version");
         let call = calls.remove(0);
@@ -267,7 +267,7 @@ impl MockJira {
             .push(MockCall::new(ret, vec![key, comment]));
     }
 
-    pub fn mock_add_version(&self, proj: &str, version: &str, ret: Result<()>) {
+    pub fn mock_add_version(&self, proj: &str, version: &str, ret: Result<Version>) {
         self.add_version_calls
             .lock()
             .unwrap()