diff --git a/src/cli/autocorrect.rs b/src/cli/autocorrect.rs index 85047b0..e8dffd3 100644 --- a/src/cli/autocorrect.rs +++ b/src/cli/autocorrect.rs @@ -193,6 +193,7 @@ const COMMAND_FLAGS: &[(&str, &[&str])] = &[ "--as-of", "--explain-score", "--include-bots", + "--include-closed", "--all-history", ], ), diff --git a/src/cli/commands/who.rs b/src/cli/commands/who.rs index 35de8e1..c105685 100644 --- a/src/cli/commands/who.rs +++ b/src/cli/commands/who.rs @@ -344,7 +344,14 @@ pub fn run_who(config: &Config, args: &WhoArgs) -> Result { .map(resolve_since_required) .transpose()?; let limit = usize::from(args.limit); - let result = query_workload(&conn, username, project_id, since_ms, limit)?; + let result = query_workload( + &conn, + username, + project_id, + since_ms, + limit, + args.include_closed, + )?; Ok(WhoRun { resolved_input: WhoResolvedInput { mode: "workload".to_string(), @@ -377,7 +384,7 @@ pub fn run_who(config: &Config, args: &WhoArgs) -> Result { WhoMode::Active => { let since_ms = resolve_since(args.since.as_deref(), "7d")?; let limit = usize::from(args.limit); - let result = query_active(&conn, project_id, since_ms, limit)?; + let result = query_active(&conn, project_id, since_ms, limit, args.include_closed)?; Ok(WhoRun { resolved_input: WhoResolvedInput { mode: "active".to_string(), @@ -1149,6 +1156,7 @@ fn query_workload( project_id: Option, since_ms: Option, limit: usize, + include_closed: bool, ) -> Result { let limit_plus_one = (limit + 1) as i64; @@ -1245,7 +1253,14 @@ fn query_workload( .collect::, _>>()?; // Query 4: Unresolved discussions where user participated - let disc_sql = "SELECT d.noteable_type, + let state_filter = if include_closed { + "" + } else { + " AND (i.id IS NULL OR i.state = 'opened') + AND (m.id IS NULL OR m.state = 'opened')" + }; + let disc_sql = format!( + "SELECT d.noteable_type, COALESCE(i.iid, m.iid) AS entity_iid, (p.path_with_namespace || CASE WHEN d.noteable_type = 'MergeRequest' THEN '!' ELSE '#' END || @@ -1266,10 +1281,12 @@ fn query_workload( ) AND (?2 IS NULL OR d.project_id = ?2) AND (?3 IS NULL OR d.last_note_at >= ?3) + {state_filter} ORDER BY d.last_note_at DESC - LIMIT ?4"; + LIMIT ?4" + ); - let mut stmt = conn.prepare_cached(disc_sql)?; + let mut stmt = conn.prepare_cached(&disc_sql)?; let unresolved_discussions: Vec = stmt .query_map( rusqlite::params![username, project_id, since_ms, limit_plus_one], @@ -1451,35 +1468,63 @@ fn query_active( project_id: Option, since_ms: i64, limit: usize, + include_closed: bool, ) -> Result { let limit_plus_one = (limit + 1) as i64; - // Total unresolved count -- two static variants - let total_sql_global = "SELECT COUNT(*) FROM discussions d - WHERE d.resolvable = 1 AND d.resolved = 0 - AND d.last_note_at >= ?1"; - let total_sql_scoped = "SELECT COUNT(*) FROM discussions d - WHERE d.resolvable = 1 AND d.resolved = 0 - AND d.last_note_at >= ?1 - AND d.project_id = ?2"; - - let total_unresolved_in_window: u32 = match project_id { - None => conn.query_row(total_sql_global, rusqlite::params![since_ms], |row| { - row.get(0) - })?, - Some(pid) => conn.query_row(total_sql_scoped, rusqlite::params![since_ms, pid], |row| { - row.get(0) - })?, + // State filter for open-entities-only (default behavior) + let state_joins = if include_closed { + "" + } else { + " LEFT JOIN issues i ON d.issue_id = i.id + LEFT JOIN merge_requests m ON d.merge_request_id = m.id" + }; + let state_filter = if include_closed { + "" + } else { + " AND (i.id IS NULL OR i.state = 'opened') + AND (m.id IS NULL OR m.state = 'opened')" }; - // Active discussions with context -- two static SQL variants - let sql_global = " + // Total unresolved count -- conditionally built + let total_sql_global = format!( + "SELECT COUNT(*) FROM discussions d + {state_joins} + WHERE d.resolvable = 1 AND d.resolved = 0 + AND d.last_note_at >= ?1 + {state_filter}" + ); + let total_sql_scoped = format!( + "SELECT COUNT(*) FROM discussions d + {state_joins} + WHERE d.resolvable = 1 AND d.resolved = 0 + AND d.last_note_at >= ?1 + AND d.project_id = ?2 + {state_filter}" + ); + + let total_unresolved_in_window: u32 = match project_id { + None => conn.query_row(&total_sql_global, rusqlite::params![since_ms], |row| { + row.get(0) + })?, + Some(pid) => { + conn.query_row(&total_sql_scoped, rusqlite::params![since_ms, pid], |row| { + row.get(0) + })? + } + }; + + // Active discussions with context -- conditionally built SQL + let sql_global = format!( + " WITH picked AS ( SELECT d.id, d.noteable_type, d.issue_id, d.merge_request_id, d.project_id, d.last_note_at FROM discussions d + {state_joins} WHERE d.resolvable = 1 AND d.resolved = 0 AND d.last_note_at >= ?1 + {state_filter} ORDER BY d.last_note_at DESC LIMIT ?2 ), @@ -1520,16 +1565,20 @@ fn query_active( LEFT JOIN note_counts nc ON nc.discussion_id = p.id LEFT JOIN participants pa ON pa.discussion_id = p.id ORDER BY p.last_note_at DESC - "; + " + ); - let sql_scoped = " + let sql_scoped = format!( + " WITH picked AS ( SELECT d.id, d.noteable_type, d.issue_id, d.merge_request_id, d.project_id, d.last_note_at FROM discussions d + {state_joins} WHERE d.resolvable = 1 AND d.resolved = 0 AND d.last_note_at >= ?1 AND d.project_id = ?2 + {state_filter} ORDER BY d.last_note_at DESC LIMIT ?3 ), @@ -1570,7 +1619,8 @@ fn query_active( LEFT JOIN note_counts nc ON nc.discussion_id = p.id LEFT JOIN participants pa ON pa.discussion_id = p.id ORDER BY p.last_note_at DESC - "; + " + ); // Row-mapping closure shared between both variants let map_row = |row: &rusqlite::Row| -> rusqlite::Result { @@ -1613,12 +1663,12 @@ fn query_active( // Select variant first, then prepare exactly one statement let discussions: Vec = match project_id { None => { - let mut stmt = conn.prepare_cached(sql_global)?; + let mut stmt = conn.prepare_cached(&sql_global)?; stmt.query_map(rusqlite::params![since_ms, limit_plus_one], &map_row)? .collect::, _>>()? } Some(pid) => { - let mut stmt = conn.prepare_cached(sql_scoped)?; + let mut stmt = conn.prepare_cached(&sql_scoped)?; stmt.query_map(rusqlite::params![since_ms, pid, limit_plus_one], &map_row)? .collect::, _>>()? } diff --git a/src/cli/commands/who_tests.rs b/src/cli/commands/who_tests.rs index 9c267cb..1707963 100644 --- a/src/cli/commands/who_tests.rs +++ b/src/cli/commands/who_tests.rs @@ -54,15 +54,27 @@ fn insert_mr(conn: &Connection, id: i64, project_id: i64, iid: i64, author: &str } fn insert_issue(conn: &Connection, id: i64, project_id: i64, iid: i64, author: &str) { + insert_issue_with_state(conn, id, project_id, iid, author, "opened"); +} + +fn insert_issue_with_state( + conn: &Connection, + id: i64, + project_id: i64, + iid: i64, + author: &str, + state: &str, +) { conn.execute( "INSERT INTO issues (id, gitlab_id, project_id, iid, title, state, author_username, created_at, updated_at, last_seen_at) - VALUES (?1, ?2, ?3, ?4, ?5, 'opened', ?6, ?7, ?8, ?9)", + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10)", rusqlite::params![ id, id * 10, project_id, iid, format!("Issue {iid}"), + state, author, now_ms(), now_ms(), @@ -134,6 +146,24 @@ fn insert_diffnote( .unwrap(); } +fn insert_note(conn: &Connection, id: i64, discussion_id: i64, project_id: i64, author: &str) { + conn.execute( + "INSERT INTO notes (id, gitlab_id, discussion_id, project_id, note_type, is_system, author_username, body, created_at, updated_at, last_seen_at) + VALUES (?1, ?2, ?3, ?4, 'DiscussionNote', 0, ?5, 'comment', ?6, ?7, ?8)", + rusqlite::params![ + id, + id * 10, + discussion_id, + project_id, + author, + now_ms(), + now_ms(), + now_ms() + ], + ) + .unwrap(); +} + fn insert_assignee(conn: &Connection, issue_id: i64, username: &str) { conn.execute( "INSERT INTO issue_assignees (issue_id, username) VALUES (?1, ?2)", @@ -263,6 +293,7 @@ fn test_is_file_path_discrimination() { as_of: None, explain_score: false, include_bots: false, + include_closed: false, all_history: false, }) .unwrap(), @@ -286,6 +317,7 @@ fn test_is_file_path_discrimination() { as_of: None, explain_score: false, include_bots: false, + include_closed: false, all_history: false, }) .unwrap(), @@ -309,6 +341,7 @@ fn test_is_file_path_discrimination() { as_of: None, explain_score: false, include_bots: false, + include_closed: false, all_history: false, }) .unwrap(), @@ -332,6 +365,7 @@ fn test_is_file_path_discrimination() { as_of: None, explain_score: false, include_bots: false, + include_closed: false, all_history: false, }) .unwrap(), @@ -355,6 +389,7 @@ fn test_is_file_path_discrimination() { as_of: None, explain_score: false, include_bots: false, + include_closed: false, all_history: false, }) .unwrap(), @@ -378,6 +413,7 @@ fn test_is_file_path_discrimination() { as_of: None, explain_score: false, include_bots: false, + include_closed: false, all_history: false, }) .unwrap(), @@ -402,6 +438,7 @@ fn test_detail_rejected_outside_expert_mode() { as_of: None, explain_score: false, include_bots: false, + include_closed: false, all_history: false, }; let mode = resolve_mode(&args).unwrap(); @@ -430,6 +467,7 @@ fn test_detail_allowed_in_expert_mode() { as_of: None, explain_score: false, include_bots: false, + include_closed: false, all_history: false, }; let mode = resolve_mode(&args).unwrap(); @@ -579,7 +617,7 @@ fn test_workload_query() { insert_assignee(&conn, 1, "dev_a"); insert_mr(&conn, 1, 1, 100, "dev_a", "opened"); - let result = query_workload(&conn, "dev_a", None, None, 20).unwrap(); + let result = query_workload(&conn, "dev_a", None, None, 20, true).unwrap(); assert_eq!(result.assigned_issues.len(), 1); assert_eq!(result.authored_mrs.len(), 1); } @@ -626,7 +664,7 @@ fn test_active_query() { // Second note by same participant -- note_count should be 2, participants still ["reviewer_b"] insert_diffnote(&conn, 2, 1, 1, "reviewer_b", "src/foo.rs", "follow-up"); - let result = query_active(&conn, None, 0, 20).unwrap(); + let result = query_active(&conn, None, 0, 20, true).unwrap(); assert_eq!(result.total_unresolved_in_window, 1); assert_eq!(result.discussions.len(), 1); assert_eq!(result.discussions[0].participants, vec!["reviewer_b"]); @@ -878,7 +916,7 @@ fn test_active_participants_sorted() { insert_diffnote(&conn, 1, 1, 1, "zebra_user", "src/foo.rs", "note 1"); insert_diffnote(&conn, 2, 1, 1, "alpha_user", "src/foo.rs", "note 2"); - let result = query_active(&conn, None, 0, 20).unwrap(); + let result = query_active(&conn, None, 0, 20, true).unwrap(); assert_eq!( result.discussions[0].participants, vec!["alpha_user", "zebra_user"] @@ -3265,3 +3303,94 @@ fn test_deterministic_accumulation_order() { ); } } + +// ─── Tests: include_closed filter ──────────────────────────────────────────── + +#[test] +fn workload_excludes_closed_entity_discussions() { + let conn = setup_test_db(); + insert_project(&conn, 1, "group/repo"); + + // Open issue with unresolved discussion + insert_issue_with_state(&conn, 10, 1, 10, "someone", "opened"); + insert_discussion(&conn, 100, 1, None, Some(10), true, false); + insert_note(&conn, 1000, 100, 1, "alice"); + + // Closed issue with unresolved discussion + insert_issue_with_state(&conn, 20, 1, 20, "someone", "closed"); + insert_discussion(&conn, 200, 1, None, Some(20), true, false); + insert_note(&conn, 2000, 200, 1, "alice"); + + // Default: exclude closed + let result = query_workload(&conn, "alice", None, None, 50, false).unwrap(); + assert_eq!(result.unresolved_discussions.len(), 1); + assert_eq!(result.unresolved_discussions[0].entity_iid, 10); +} + +#[test] +fn workload_include_closed_flag_shows_all() { + let conn = setup_test_db(); + insert_project(&conn, 1, "group/repo"); + + insert_issue_with_state(&conn, 10, 1, 10, "someone", "opened"); + insert_discussion(&conn, 100, 1, None, Some(10), true, false); + insert_note(&conn, 1000, 100, 1, "alice"); + + insert_issue_with_state(&conn, 20, 1, 20, "someone", "closed"); + insert_discussion(&conn, 200, 1, None, Some(20), true, false); + insert_note(&conn, 2000, 200, 1, "alice"); + + let result = query_workload(&conn, "alice", None, None, 50, true).unwrap(); + assert_eq!(result.unresolved_discussions.len(), 2); +} + +#[test] +fn workload_excludes_merged_mr_discussions() { + let conn = setup_test_db(); + insert_project(&conn, 1, "group/repo"); + + // Open MR with unresolved discussion + insert_mr(&conn, 10, 1, 10, "someone", "opened"); + insert_discussion(&conn, 100, 1, Some(10), None, true, false); + insert_note(&conn, 1000, 100, 1, "alice"); + + // Merged MR with unresolved discussion + insert_mr(&conn, 20, 1, 20, "someone", "merged"); + insert_discussion(&conn, 200, 1, Some(20), None, true, false); + insert_note(&conn, 2000, 200, 1, "alice"); + + let result = query_workload(&conn, "alice", None, None, 50, false).unwrap(); + assert_eq!(result.unresolved_discussions.len(), 1); + assert_eq!(result.unresolved_discussions[0].entity_iid, 10); + + // include_closed shows both + let result = query_workload(&conn, "alice", None, None, 50, true).unwrap(); + assert_eq!(result.unresolved_discussions.len(), 2); +} + +#[test] +fn active_excludes_closed_entity_discussions() { + let conn = setup_test_db(); + insert_project(&conn, 1, "group/repo"); + + // Open issue with unresolved discussion + insert_issue_with_state(&conn, 10, 1, 10, "someone", "opened"); + insert_discussion(&conn, 100, 1, None, Some(10), true, false); + insert_note(&conn, 1000, 100, 1, "alice"); + + // Closed issue with unresolved discussion + insert_issue_with_state(&conn, 20, 1, 20, "someone", "closed"); + insert_discussion(&conn, 200, 1, None, Some(20), true, false); + insert_note(&conn, 2000, 200, 1, "alice"); + + // Default: exclude closed + let result = query_active(&conn, None, 0, 50, false).unwrap(); + assert_eq!(result.discussions.len(), 1); + assert_eq!(result.discussions[0].entity_iid, 10); + assert_eq!(result.total_unresolved_in_window, 1); + + // include_closed shows both + let result = query_active(&conn, None, 0, 50, true).unwrap(); + assert_eq!(result.discussions.len(), 2); + assert_eq!(result.total_unresolved_in_window, 2); +} diff --git a/src/cli/mod.rs b/src/cli/mod.rs index c6d740b..08d81a9 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -964,6 +964,10 @@ pub struct WhoArgs { #[arg(long = "include-bots", help_heading = "Scoring")] pub include_bots: bool, + /// Include discussions on closed issues and merged/closed MRs + #[arg(long, help_heading = "Filters")] + pub include_closed: bool, + /// Remove the default time window (query all history). Conflicts with --since. #[arg( long = "all-history",