diff --git a/Cargo.lock b/Cargo.lock index 2a1d361..a2809cf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -926,7 +926,7 @@ checksum = "40ecd4077b5ae9fd2e9e169b102c6c330d0605168eb0e8bf79952b256dbefffd" [[package]] name = "git-ar" -version = "0.1.89" +version = "0.1.90" dependencies = [ "anyhow", "chrono", diff --git a/Cargo.toml b/Cargo.toml index d731328..e29c310 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "git-ar" -version = "0.1.89" +version = "0.1.90" edition = "2021" license = "MIT" diff --git a/src/cli/my.rs b/src/cli/my.rs index cb71964..81b04dd 100644 --- a/src/cli/my.rs +++ b/src/cli/my.rs @@ -1,7 +1,9 @@ use clap::Parser; use crate::cmds::{ - gist::GistListCliArgs, merge_request::MergeRequestListCliArgs, project::ProjectListCliArgs, + gist::GistListCliArgs, + merge_request::{MergeRequestListCliArgs, MergeRequestUser}, + project::ProjectListCliArgs, }; use super::{common::ListArgs, merge_request::ListMergeRequest, project::ListProject}; @@ -14,8 +16,11 @@ pub struct MyCommand { #[derive(Parser)] enum MySubcommand { - #[clap(about = "Lists your assigned merge requests", name = "mr")] - MergeRequest(ListMergeRequest), + #[clap( + about = "Lists the merge requests where you are the author, assignee or the reviewer", + name = "mr" + )] + MergeRequest(ListMyMergeRequest), #[clap(about = "Lists your projects", name = "pj")] Project(ListProject), #[clap(about = "Lists your starred projects", name = "st")] @@ -24,6 +29,22 @@ enum MySubcommand { Gist(ListGist), } +#[derive(Parser)] +struct ListMyMergeRequest { + /// Filter merge requests where you are the assignee. Gitlab and Github. + #[clap(long, group = "merge_request")] + assignee: bool, + /// Filter merge requests where you are the author. Default if none + /// provided. Gitlab and Github. + #[clap(long, group = "merge_request")] + author: bool, + /// Filter merge requests where you are the reviewer. Gitlab only. + #[clap(long, group = "merge_request")] + reviewer: bool, + #[clap(flatten)] + list_merge_request: ListMergeRequest, +} + pub enum MyOptions { MergeRequest(MergeRequestListCliArgs), Project(ProjectListCliArgs), @@ -41,12 +62,33 @@ impl From for MyOptions { } } -impl From for MyOptions { - fn from(options: ListMergeRequest) -> Self { - MyOptions::MergeRequest(MergeRequestListCliArgs::new( - options.state.into(), - options.list_args.into(), - )) +impl From for MyOptions { + fn from(options: ListMyMergeRequest) -> Self { + MyOptions::MergeRequest( + MergeRequestListCliArgs::builder() + .state(options.list_merge_request.state.into()) + .list_args(options.list_merge_request.list_args.into()) + .assignee(if options.assignee { + Some(MergeRequestUser::Me) + } else { + None + }) + // Author is the default if none is provided. + .author( + if options.author || (!options.assignee && !options.reviewer) { + Some(MergeRequestUser::Me) + } else { + None + }, + ) + .reviewer(if options.reviewer { + Some(MergeRequestUser::Me) + } else { + None + }) + .build() + .unwrap(), + ) } } @@ -111,7 +153,88 @@ mod tests { Command::My(MyCommand { subcommand: MySubcommand::MergeRequest(options), }) => { - assert_eq!(options.state, MergeRequestStateStateCli::Opened); + assert_eq!( + options.list_merge_request.state, + MergeRequestStateStateCli::Opened + ); + options + } + _ => panic!("Expected MyCommand"), + }; + let options: MyOptions = my_command.into(); + match options { + MyOptions::MergeRequest(options) => { + assert_eq!(options.state, MergeRequestState::Opened); + assert_eq!(options.author, Some(MergeRequestUser::Me)); + } + _ => panic!("Expected MyOptions::MergeRequest"), + } + } + + #[test] + fn test_my_merge_request_cli_args_reviewer() { + let args = Args::parse_from(vec!["gr", "my", "mr", "opened", "--reviewer"]); + let my_command = match args.command { + Command::My(MyCommand { + subcommand: MySubcommand::MergeRequest(options), + }) => { + assert_eq!( + options.list_merge_request.state, + MergeRequestStateStateCli::Opened + ); + assert!(options.reviewer); + options + } + _ => panic!("Expected MyCommand"), + }; + let options: MyOptions = my_command.into(); + match options { + MyOptions::MergeRequest(options) => { + assert_eq!(options.state, MergeRequestState::Opened); + assert_eq!(options.reviewer, Some(MergeRequestUser::Me)); + } + _ => panic!("Expected MyOptions::MergeRequest"), + } + } + + #[test] + fn test_my_merge_request_cli_args_author() { + let args = Args::parse_from(vec!["gr", "my", "mr", "opened", "--author"]); + let my_command = match args.command { + Command::My(MyCommand { + subcommand: MySubcommand::MergeRequest(options), + }) => { + assert_eq!( + options.list_merge_request.state, + MergeRequestStateStateCli::Opened + ); + assert!(options.author); + options + } + _ => panic!("Expected MyCommand"), + }; + let options: MyOptions = my_command.into(); + match options { + MyOptions::MergeRequest(options) => { + assert_eq!(options.state, MergeRequestState::Opened); + assert_eq!(options.author, Some(MergeRequestUser::Me)); + } + _ => panic!("Expected MyOptions::MergeRequest"), + } + } + + #[test] + fn test_my_merge_request_cli_args_assignee() { + let args = Args::parse_from(vec!["gr", "my", "mr", "opened", "--assignee"]); + let my_command = match args.command { + Command::My(MyCommand { + subcommand: MySubcommand::MergeRequest(options), + }) => { + assert_eq!( + options.list_merge_request.state, + MergeRequestStateStateCli::Opened + ); + assert!(options.assignee); options } _ => panic!("Expected MyCommand"), @@ -120,6 +243,7 @@ mod tests { match options { MyOptions::MergeRequest(options) => { assert_eq!(options.state, MergeRequestState::Opened); + assert_eq!(options.assignee, Some(MergeRequestUser::Me)); } _ => panic!("Expected MyOptions::MergeRequest"), } diff --git a/src/cmds/common.rs b/src/cmds/common.rs index 3ac859e..5c96e11 100644 --- a/src/cmds/common.rs +++ b/src/cmds/common.rs @@ -1,8 +1,9 @@ -use crate::display; -use crate::remote::MergeRequestListBodyArgs; +use crate::config::Config; +use crate::remote::{Member, MergeRequestListBodyArgs}; /// Common functions and macros that are used by multiple commands use crate::Result; use crate::{api_traits::MergeRequest, remote::ListRemoteCliArgs}; +use crate::{display, remote}; use std::fmt::Display; use std::io::Write; use std::sync::Arc; @@ -239,3 +240,19 @@ list_resource!( ); list_resource!(list_trending, TrendingProjectURL, String, TrendingCliArgs); + +pub fn get_user( + domain: &str, + path: &str, + config: &Arc, + cli_args: &ListRemoteCliArgs, +) -> Result { + let remote = remote::get_auth_user( + domain.to_string(), + path.to_string(), + config.clone(), + cli_args.get_args.refresh_cache, + )?; + let user = remote.get()?; + Ok(user) +} diff --git a/src/cmds/merge_request.rs b/src/cmds/merge_request.rs index ca6bdef..e089547 100644 --- a/src/cmds/merge_request.rs +++ b/src/cmds/merge_request.rs @@ -18,7 +18,7 @@ use std::{ sync::Arc, }; -use super::common; +use super::common::{self, get_user}; #[derive(Builder, Clone)] pub struct MergeRequestCliArgs { @@ -49,9 +49,26 @@ impl MergeRequestCliArgs { } } +/// Enum for filtering merge requests by user +/// Me: current authenticated user +/// Other: another username, provided by cli flags. +#[derive(Clone, Debug, PartialEq)] +pub enum MergeRequestUser { + Me, + Other(String), +} + +#[derive(Builder)] pub struct MergeRequestListCliArgs { pub state: MergeRequestState, pub list_args: ListRemoteCliArgs, + // Filtering options. Make use of the builder pattern. + #[builder(default)] + pub assignee: Option, + #[builder(default)] + pub author: Option, + #[builder(default)] + pub reviewer: Option, } impl MergeRequestListCliArgs { @@ -59,8 +76,14 @@ impl MergeRequestListCliArgs { MergeRequestListCliArgs { state, list_args: args, + assignee: None, + author: None, + reviewer: None, } } + pub fn builder() -> MergeRequestListCliArgsBuilder { + MergeRequestListCliArgsBuilder::default() + } } #[derive(Builder)] @@ -201,9 +224,7 @@ pub fn execute( let mr_body = get_repo_project_info(cmds)?; open(mr_remote, config, mr_body, &cli_args) } - MergeRequestOptions::List(cli_args) => { - list_merge_requests(domain, path, config, cli_args, None) - } + MergeRequestOptions::List(cli_args) => list_merge_requests(domain, path, config, cli_args), MergeRequestOptions::Merge { id } => { let remote = remote::get_mr(domain, path, config, false)?; merge(remote, id) @@ -276,24 +297,69 @@ pub fn get_reader_file_cli(file_path: &str) -> Result, + domain: &str, + path: &str, + config: &Arc, + list_args: &ListRemoteCliArgs, +) -> Result> { + let member = match user { + Some(MergeRequestUser::Me) => Some(get_user(domain, path, config, list_args)?), + // TODO filter by specific username, not necessarily the + // authenticated user. + _ => None, + }; + Ok(member) +} + pub fn list_merge_requests( domain: String, path: String, config: Arc, cli_args: MergeRequestListCliArgs, - assignee_id: Option, ) -> Result<()> { + // Author, assignee and reviewer are mutually exclusive filters checked on + // cli's flags. While we do sequential calls to retrieve them it is a very + // fast operation. Only one ends up calling the remote to retrieve it's id. + let author = get_filter_user( + &cli_args.author, + &domain, + &path, + &config, + &cli_args.list_args, + )?; + + let assignee = get_filter_user( + &cli_args.assignee, + &domain, + &path, + &config, + &cli_args.list_args, + )?; + + let reviewer = get_filter_user( + &cli_args.reviewer, + &domain, + &path, + &config, + &cli_args.list_args, + )?; + let remote = remote::get_mr( domain, path, config, cli_args.list_args.get_args.refresh_cache, )?; + let from_to_args = remote::validate_from_to_page(&cli_args.list_args)?; let body_args = MergeRequestListBodyArgs::builder() .list_args(from_to_args) .state(cli_args.state) - .assignee_id(assignee_id) + .assignee(assignee) + .author(author) + .reviewer(reviewer) .build()?; if cli_args.list_args.num_pages { return common::num_merge_request_pages(remote, body_args, std::io::stdout()); @@ -810,7 +876,7 @@ mod tests { let body_args = MergeRequestListBodyArgs::builder() .list_args(None) .state(MergeRequestState::Opened) - .assignee_id(None) + .assignee(None) .build() .unwrap(); let cli_args = MergeRequestListCliArgs::new( @@ -832,7 +898,7 @@ mod tests { let body_args = MergeRequestListBodyArgs::builder() .list_args(None) .state(MergeRequestState::Opened) - .assignee_id(None) + .assignee(None) .build() .unwrap(); let cli_args = MergeRequestListCliArgs::new( @@ -850,7 +916,7 @@ mod tests { let body_args = MergeRequestListBodyArgs::builder() .list_args(None) .state(MergeRequestState::Opened) - .assignee_id(None) + .assignee(None) .build() .unwrap(); let cli_args = MergeRequestListCliArgs::new( @@ -880,7 +946,7 @@ mod tests { let body_args = MergeRequestListBodyArgs::builder() .list_args(None) .state(MergeRequestState::Opened) - .assignee_id(None) + .assignee(None) .build() .unwrap(); let cli_args = MergeRequestListCliArgs::new( diff --git a/src/cmds/my.rs b/src/cmds/my.rs index a4a9053..5c5c6c4 100644 --- a/src/cmds/my.rs +++ b/src/cmds/my.rs @@ -1,15 +1,10 @@ use std::{io::Write, sync::Arc}; -use crate::{ - api_traits::RemoteProject, - cli::my::MyOptions, - config::Config, - remote::{self, ListRemoteCliArgs, Member}, - Result, -}; +use crate::{api_traits::RemoteProject, cli::my::MyOptions, config::Config, remote, Result}; use super::{ - common, gist, merge_request, + common::{self, get_user}, + gist, merge_request, project::{ProjectListBodyArgs, ProjectListCliArgs}, }; @@ -21,8 +16,7 @@ pub fn execute( ) -> Result<()> { match options { MyOptions::MergeRequest(cli_args) => { - let user = get_user(&domain, &path, &config, &cli_args.list_args)?; - merge_request::list_merge_requests(domain, path, config, cli_args, Some(user.id)) + merge_request::list_merge_requests(domain, path, config, cli_args) } MyOptions::Project(cli_args) => { let user = get_user(&domain, &path, &config, &cli_args.list_args)?; @@ -68,22 +62,6 @@ pub fn execute( } } -fn get_user( - domain: &str, - path: &str, - config: &Arc, - cli_args: &ListRemoteCliArgs, -) -> Result { - let remote = remote::get_auth_user( - domain.to_string(), - path.to_string(), - config.clone(), - cli_args.get_args.refresh_cache, - )?; - let user = remote.get()?; - Ok(user) -} - fn list_user_projects( remote: Arc, body_args: ProjectListBodyArgs, @@ -95,6 +73,8 @@ fn list_user_projects( #[cfg(test)] mod tests { + use remote::Member; + use crate::cmds::project::ProjectListCliArgs; use self::remote::{ListRemoteCliArgs, Project}; diff --git a/src/github/merge_request.rs b/src/github/merge_request.rs index d79d95b..e8a7857 100644 --- a/src/github/merge_request.rs +++ b/src/github/merge_request.rs @@ -25,8 +25,16 @@ impl Github { // pull request is considered closed. MergeRequestState::Closed | MergeRequestState::Merged => "closed".to_string(), }; - if args.assignee_id.is_some() { - return format!("{}/issues?state={}", self.rest_api_basepath, state); + if args.assignee.is_some() { + return format!( + "{}/issues?state={}&filter=assigned", + self.rest_api_basepath, state + ); + } else if args.author.is_some() { + return format!( + "{}/issues?state={}&filter=created", + self.rest_api_basepath, state + ); } format!( "{}/repos/{}/pulls?state={}", @@ -212,7 +220,7 @@ impl> MergeRequest for Github { None, ApiOperation::MergeRequest, ); - if args.assignee_id.is_some() { + if args.assignee.is_some() || args.author.is_some() { // Pull requests for the current authenticated user. // Filter those reponses that have pull_request not empty See ref: // https://docs.github.com/en/rest/issues/issues?apiVersion=2022-11-28#list-issues-assigned-to-the-authenticated-user @@ -484,7 +492,7 @@ mod test { use crate::{ http::{self, Headers}, - remote::{ListBodyArgs, MergeRequestState}, + remote::{ListBodyArgs, Member, MergeRequestState}, setup_client, test::utils::{ default_github, get_contract, BasePath, ClientType, ContractType, Domain, @@ -681,7 +689,7 @@ mod test { let args = MergeRequestListBodyArgs::builder() .state(MergeRequestState::Opened) .list_args(None) - .assignee_id(None) + .assignee(None) .build() .unwrap(); assert_eq!(Some(2), github.num_pages(args).unwrap()); @@ -710,7 +718,7 @@ mod test { .build() .unwrap(), )) - .assignee_id(None) + .assignee(None) .build() .unwrap(); github.list(args).unwrap(); @@ -725,7 +733,40 @@ mod test { } #[test] - fn test_get_pull_requests_for_auth_user() { + fn test_get_pull_requests_for_auth_user_is_assignee() { + let contracts = ResponseContracts::new(ContractType::Github).add_contract( + 200, + "list_issues_user.json", + None, + ); + let (client, github) = setup_client!(contracts, default_github(), dyn MergeRequest); + let args = MergeRequestListBodyArgs::builder() + .state(MergeRequestState::Opened) + .list_args(None) + .assignee(Some( + Member::builder() + .name("tom".to_string()) + .username("tsawyer".to_string()) + .id(123456) + .build() + .unwrap(), + )) + .build() + .unwrap(); + let merge_requests = github.list(args).unwrap(); + assert_eq!( + "https://api.github.com/issues?state=open&filter=assigned", + *client.url() + ); + assert!(merge_requests.len() == 1); + assert_eq!( + Some(ApiOperation::MergeRequest), + *client.api_operation.borrow() + ); + } + + #[test] + fn test_get_pull_requests_for_auth_user_is_author() { let contracts = ResponseContracts::new(ContractType::Github).add_contract( 200, "list_issues_user.json", @@ -735,11 +776,21 @@ mod test { let args = MergeRequestListBodyArgs::builder() .state(MergeRequestState::Opened) .list_args(None) - .assignee_id(Some(123456)) + .author(Some( + Member::builder() + .name("tom".to_string()) + .username("tsawyer".to_string()) + .id(12345) + .build() + .unwrap(), + )) .build() .unwrap(); let merge_requests = github.list(args).unwrap(); - assert_eq!("https://api.github.com/issues?state=open", *client.url()); + assert_eq!( + "https://api.github.com/issues?state=open&filter=created", + *client.url() + ); assert!(merge_requests.len() == 1); assert_eq!( Some(ApiOperation::MergeRequest), diff --git a/src/gitlab/merge_request.rs b/src/gitlab/merge_request.rs index a2d595c..db81312 100644 --- a/src/gitlab/merge_request.rs +++ b/src/gitlab/merge_request.rs @@ -200,10 +200,20 @@ impl> MergeRequest for Gitlab { impl Gitlab { fn list_merge_request_url(&self, args: &MergeRequestListBodyArgs, num_pages: bool) -> String { - let mut url = if let Some(assignee_id) = args.assignee_id { + let mut url = if let Some(assignee) = &args.assignee { format!( "{}?state={}&assignee_id={}", - self.merge_requests_url, args.state, assignee_id + self.merge_requests_url, args.state, assignee.id + ) + } else if let Some(reviewer) = &args.reviewer { + format!( + "{}?state={}&reviewer_id={}", + self.merge_requests_url, args.state, reviewer.id + ) + } else if let Some(author) = &args.author { + format!( + "{}?state={}&author_id={}", + self.merge_requests_url, args.state, author.id ) } else { format!( @@ -375,7 +385,7 @@ impl From for Comment { #[cfg(test)] mod test { - use crate::remote::{ListBodyArgs, MergeRequestState}; + use crate::remote::{ListBodyArgs, Member, MergeRequestState}; use crate::setup_client; use crate::test::utils::{ default_gitlab, get_contract, BasePath, ClientType, ContractType, Domain, ResponseContracts, @@ -397,7 +407,7 @@ mod test { .build() .unwrap(), )) - .assignee_id(None) + .assignee(None) .build() .unwrap(); gitlab.list(args).unwrap(); @@ -414,7 +424,14 @@ mod test { let args = MergeRequestListBodyArgs::builder() .state(MergeRequestState::Opened) .list_args(None) - .assignee_id(Some(1234)) + .assignee(Some( + Member::builder() + .name("tom".to_string()) + .username("tsawyer".to_string()) + .id(1234) + .build() + .unwrap(), + )) .build() .unwrap(); gitlab.list(args).unwrap(); @@ -424,6 +441,54 @@ mod test { ); } + #[test] + fn test_list_all_merge_requests_auth_user_is_reviewer() { + let contract = ResponseContracts::new(ContractType::Gitlab).add_body(200, Some("[]"), None); + let (client, gitlab) = setup_client!(contract, default_gitlab(), dyn MergeRequest); + let args = MergeRequestListBodyArgs::builder() + .state(MergeRequestState::Opened) + .list_args(None) + .reviewer(Some( + Member::builder() + .name("tom".to_string()) + .username("tsawyer".to_string()) + .id(123) + .build() + .unwrap(), + )) + .build() + .unwrap(); + gitlab.list(args).unwrap(); + assert_eq!( + "https://gitlab.com/api/v4/merge_requests?state=opened&reviewer_id=123", + *client.url(), + ); + } + + #[test] + fn test_list_all_merge_requests_auth_user_is_the_author() { + let contract = ResponseContracts::new(ContractType::Gitlab).add_body(200, Some("[]"), None); + let (client, gitlab) = setup_client!(contract, default_gitlab(), dyn MergeRequest); + let args = MergeRequestListBodyArgs::builder() + .state(MergeRequestState::Opened) + .list_args(None) + .author(Some( + Member::builder() + .name("tom".to_string()) + .username("tsawyer".to_string()) + .id(192) + .build() + .unwrap(), + )) + .build() + .unwrap(); + gitlab.list(args).unwrap(); + assert_eq!( + "https://gitlab.com/api/v4/merge_requests?state=opened&author_id=192", + *client.url(), + ); + } + #[test] fn test_open_merge_request() { let mr_args = MergeRequestBodyArgs::builder().build().unwrap(); @@ -523,7 +588,7 @@ mod test { let body_args = MergeRequestListBodyArgs::builder() .state(MergeRequestState::Opened) .list_args(None) - .assignee_id(None) + .assignee(None) .build() .unwrap(); assert_eq!(Some(2), gitlab.num_pages(body_args).unwrap()); @@ -547,7 +612,14 @@ mod test { let body_args = MergeRequestListBodyArgs::builder() .state(MergeRequestState::Opened) .list_args(None) - .assignee_id(Some(1234)) + .assignee(Some( + Member::builder() + .name("tom".to_string()) + .username("tsawyer".to_string()) + .id(1234) + .build() + .unwrap(), + )) .build() .unwrap(); assert_eq!(Some(2), gitlab.num_pages(body_args).unwrap()); @@ -565,7 +637,7 @@ mod test { let body_args = MergeRequestListBodyArgs::builder() .state(MergeRequestState::Opened) .list_args(None) - .assignee_id(None) + .assignee(None) .build() .unwrap(); assert_eq!(Some(1), gitlab.num_pages(body_args).unwrap()); @@ -579,7 +651,7 @@ mod test { let body_args = MergeRequestListBodyArgs::builder() .state(MergeRequestState::Opened) .list_args(None) - .assignee_id(None) + .assignee(None) .build() .unwrap(); assert!(gitlab.num_pages(body_args).is_err()); @@ -599,7 +671,7 @@ mod test { let body_args = MergeRequestListBodyArgs::builder() .state(MergeRequestState::Opened) .list_args(None) - .assignee_id(None) + .assignee(None) .build() .unwrap(); assert_eq!(None, gitlab.num_pages(body_args).unwrap()); diff --git a/src/remote.rs b/src/remote.rs index 78a106f..9d1954f 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -273,7 +273,12 @@ impl MergeRequestBodyArgs { pub struct MergeRequestListBodyArgs { pub state: MergeRequestState, pub list_args: Option, - pub assignee_id: Option, + #[builder(default)] + pub assignee: Option, + #[builder(default)] + pub author: Option, + #[builder(default)] + pub reviewer: Option, } impl MergeRequestListBodyArgs {