fix(who): exclude self-assigned reviewers from file-change reviewer signal

Signal 4 (mr_reviewers + mr_file_changes) was missing the self-review
exclusion that signal 1 (DiffNote reviewer) already had. An MR author
listed as their own reviewer would be double-counted as both author
and reviewer, inflating their score.

Also removes redundant SELECT DISTINCT from signal 2 (GROUP BY
already ensures uniqueness).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Taylor Eernisse
2026-02-08 13:42:40 -05:00
parent 95b7183add
commit c54a969269

View File

@@ -568,7 +568,7 @@ fn query_expert(
UNION ALL
-- 2. DiffNote MR author
SELECT DISTINCT
SELECT
m.author_username AS username,
'diffnote_author' AS signal,
m.id AS mr_id,
@@ -616,6 +616,7 @@ fn query_expert(
JOIN merge_requests m ON fc.merge_request_id = m.id
JOIN mr_reviewers r ON r.merge_request_id = m.id
WHERE r.username IS NOT NULL
AND (m.author_username IS NULL OR r.username != m.author_username)
AND m.state IN ('opened','merged')
AND fc.new_path {path_op}
AND m.updated_at >= ?2
@@ -1257,6 +1258,7 @@ fn query_overlap(
JOIN projects p ON m.project_id = p.id
JOIN mr_reviewers r ON r.merge_request_id = m.id
WHERE r.username IS NOT NULL
AND (m.author_username IS NULL OR r.username != m.author_username)
AND m.state IN ('opened','merged')
AND fc.new_path {path_op}
AND m.updated_at >= ?2
@@ -2845,4 +2847,53 @@ mod tests {
assert_eq!(pq.value, "src/Dockerfile");
assert!(!pq.is_prefix);
}
#[test]
fn test_expert_excludes_self_assigned_reviewer() {
// MR author listed in mr_reviewers for their own MR should NOT be
// counted as a reviewer (same principle as DiffNote self-review exclusion)
let conn = setup_test_db();
insert_project(&conn, 1, "team/backend");
insert_mr(&conn, 1, 1, 100, "author_a", "merged");
insert_file_change(&conn, 1, 1, "src/auth/login.rs", "modified");
// author_a is self-assigned as reviewer
insert_reviewer(&conn, 1, "author_a");
// real_reviewer is also assigned
insert_reviewer(&conn, 1, "real_reviewer");
let result = query_expert(&conn, "src/auth/login.rs", None, 0, 20).unwrap();
// author_a should appear as author only, not reviewer
let author = result
.experts
.iter()
.find(|e| e.username == "author_a")
.unwrap();
assert_eq!(author.review_mr_count, 0);
assert!(author.author_mr_count > 0);
// real_reviewer should appear as reviewer
let reviewer = result
.experts
.iter()
.find(|e| e.username == "real_reviewer")
.unwrap();
assert!(reviewer.review_mr_count > 0);
}
#[test]
fn test_overlap_excludes_self_assigned_reviewer() {
// Same self-review exclusion for overlap mode via file changes
let conn = setup_test_db();
insert_project(&conn, 1, "team/backend");
insert_mr(&conn, 1, 1, 100, "author_a", "merged");
insert_file_change(&conn, 1, 1, "src/auth/login.rs", "modified");
insert_reviewer(&conn, 1, "author_a"); // self-assigned
let result = query_overlap(&conn, "src/auth/", None, 0, 20).unwrap();
let user = result.users.iter().find(|u| u.username == "author_a");
// Should appear (as author) but NOT have reviewer touch count
assert!(user.is_some());
assert_eq!(user.unwrap().review_touch_count, 0);
}
}