From bc8d33214240390d83cee187b80bf03661c2f526 Mon Sep 17 00:00:00 2001 From: Bryan Blanchard Date: Wed, 7 Feb 2024 16:20:11 -0600 Subject: [PATCH] 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 --- lib/src/jira/api.rs | 23 +++++++++------- lib/src/jira/models.rs | 8 ++++-- lib/src/jira/workflow.rs | 41 ++++++++++++++++++----------- lib/src/version.rs | 7 +++++ octobot/src/assets/app.js | 6 +++-- octobot/src/assets/versions.html | 2 +- octobot/src/server/admin.rs | 20 +++++++++++++- octobot/tests/jira_workflow_test.rs | 40 +++++++++++++++++++--------- octobot/tests/mocks/mock_jira.rs | 6 ++--- 9 files changed, 107 insertions(+), 46 deletions(-) diff --git a/lib/src/jira/api.rs b/lib/src/jira/api.rs index 3b019794..16ff4b4d 100644 --- 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; async fn get_versions(&self, proj: &str) -> Result>; 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 { #[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", &req) + .await + .map_err(|e| { + anyhow!( + "Error adding version {} to project {}: {}", + version, + proj, + e + ) + }) } async fn get_versions(&self, proj: &str) -> Result> { diff --git a/lib/src/jira/models.rs b/lib/src/jira/models.rs index 63fe569d..7870c2d6 100644 --- 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 index 0a13ac32..0cbffe1b 100644 --- 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>> { +) -> Result { 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::>(); 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 index 37700f0e..b3b470a3 100644 --- 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, } +#[derive(Clone, Debug, PartialEq)] +pub struct MergedVersion { + pub issues: HashMap>, + pub version_id: Option, +} + impl Version { pub fn parse(version_str: &str) -> Option { let parts = version_str diff --git a/octobot/src/assets/app.js b/octobot/src/assets/app.js index 192f8c72..3bb5dc50 100644 --- 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 index d41c78bb..8516ffd8 100644 --- a/octobot/src/assets/versions.html +++ b/octobot/src/assets/versions.html @@ -48,7 +48,7 @@

{{key}}

Success!

- The following JIRAs were found with pending versions were migrated to {{lastVersion}} + The following JIRAs were found with pending versions were migrated to {{lastVersion}}

{{key}}

diff --git a/octobot/src/server/admin.rs b/octobot/src/server/admin.rs index 284a6a58..2a938856 100644 --- a/octobot/src/server/admin.rs +++ b/octobot/src/server/admin.rs @@ -252,6 +252,7 @@ struct MergeVersionsResp { jira_base: String, login_suffix: Option, versions: HashMap>, + version_url: Option, error: Option, } @@ -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) -> 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 { diff --git a/octobot/tests/jira_workflow_test.rs b/octobot/tests/jira_workflow_test.rs index fbc2945c..b10c26bf 100644 --- 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 index dfbf758e..a8335944 100644 --- 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>>>, transition_issue_calls: Mutex>>, comment_issue_calls: Mutex>>, - add_version_calls: Mutex>>, + add_version_calls: Mutex>>, get_versions_calls: Mutex>>>, assign_fix_version_calls: Mutex>>, reorder_version_calls: Mutex>>, @@ -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 { 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) { self.add_version_calls .lock() .unwrap()