diff --git a/discussions.py b/discussions.py index b3ab1d0..450186f 100644 --- a/discussions.py +++ b/discussions.py @@ -31,9 +31,21 @@ def get_discussions(token: str, search_query: str, ghe: str): title url createdAt - comments(first: 1) { + author { + login + __typename + } + # Only the first 100 comments are fetched (no + # pagination). MAX_COMMENTS_EVAL defaults to 20, so + # this ceiling is not hit in practice; setting it above + # 100 would silently cap discussion mentor counts. + comments(first: 100) { nodes { createdAt + author { + login + __typename + } } } answerChosenAt diff --git a/most_active_mentors.py b/most_active_mentors.py index 51beac4..bb4ede3 100755 --- a/most_active_mentors.py +++ b/most_active_mentors.py @@ -56,8 +56,13 @@ def count_comments_per_user( Args: issue (Union[Issue, None]): A GitHub issue. + discussion (Union[dict, None]): A GitHub discussion as returned by + discussions.get_discussions (a plain GraphQL dict, not a PyGithub + object). pull_request (Union[PullRequest, None]): A GitHub pull request. + ready_for_review_at (Union[datetime, None]): When the item became + ready for review; comments before this are ignored. ignore_users (List[str]): A list of GitHub usernames to ignore. max_comments_to_eval: Maximum number of comments per item to look at. heavily_involved: Maximum number of comments to count for one @@ -118,27 +123,41 @@ def count_comments_per_user( else: mentor_count[review_comment.user.login] = 1 - # The discussion branch below is dead in production (tracked in #774): - # the GraphQL query in discussions.get_discussions fetches no comment - # author data, attribute access on dict nodes would AttributeError, - # and the ignore_comment call below passes comment.user as both - # issue_user and comment_user (self-reference always True). - if discussion and len(discussion["comments"]["nodes"]) > 0: # pragma: no cover - for comment in discussion["comments"]["nodes"]: - if ignore_comment( - comment.user, - comment.user, - ignore_users, - comment.submitted_at, - comment.ready_for_review_at, - ): - continue + # The discussion branch: use dict access because GraphQL returns plain + # dicts (not PyGithub objects). Filtering is inlined here (rather than + # reusing ignore_comment, which expects PyGithub objects): the discussion + # author's login is compared against each comment author to drop + # self-comments. + if discussion and len(discussion["comments"]["nodes"]) > 0: + discussion_author_login = (discussion.get("author") or {}).get("login", "") + comment_count = 0 + for comment in discussion["comments"]["nodes"]: + if comment_count >= max_comments_to_eval: + break + comment_count += 1 + comment_author = comment.get("author") or {} + comment_login = comment_author.get("login", "") + comment_type = comment_author.get("__typename", "") + comment_created_at = comment.get("createdAt") + + if ( + not comment_login + # ignore bots + or comment_type == "Bot" + # ignore comments by the discussion author + or comment_login == discussion_author_login + # ignore users in the ignore list + or comment_login in ignore_users + # ignore comments without a timestamp + or not comment_created_at + ): + continue - # increase the number of comments left by current user by 1 - if comment.user.login in mentor_count: - mentor_count[comment.user.login] += 1 - else: - mentor_count[comment.user.login] = 1 + if comment_login in mentor_count: + if mentor_count[comment_login] < heavily_involved: + mentor_count[comment_login] += 1 + else: + mentor_count[comment_login] = 1 return mentor_count diff --git a/test_most_active_mentors.py b/test_most_active_mentors.py index 81ef012..dad6165 100755 --- a/test_most_active_mentors.py +++ b/test_most_active_mentors.py @@ -201,3 +201,185 @@ def test_count_comments_per_user_review_limit(self): mock_issue, pull_request=mock_pr, max_comments_to_eval=5 ) self.assertEqual(result, {"reviewer": 5}) + + +class TestCountCommentsDiscussions(unittest.TestCase): + """Covers the discussion branch of count_comments_per_user. + + Before the fix for #774 this branch was dead code because: + 1. The GraphQL query fetched no comment author data. + 2. Attribute access on dict nodes would raise AttributeError. + 3. The ignore_comment call passed comment.user as both issue_user + and comment_user, so every comment was silently skipped. + """ + + def _make_discussion(self, author_login, comments): + """Return a minimal discussion dict as returned by get_discussions.""" + return { + "title": "Test discussion", + "url": "https://github.com/org/repo/discussions/1", + "createdAt": "2024-01-01T00:00:00Z", + "author": {"login": author_login, "__typename": "User"}, + "comments": {"nodes": comments}, + "answerChosenAt": None, + "closedAt": None, + } + + def test_counts_commenter(self): + """A comment by a user other than the author is counted.""" + discussion = self._make_discussion( + "op", + [ + { + "createdAt": "2024-01-02T00:00:00Z", + "author": {"login": "mentor", "__typename": "User"}, + }, + ], + ) + result = count_comments_per_user(None, discussion=discussion) + self.assertEqual(result, {"mentor": 1}) + + def test_ignores_discussion_author_self_comment(self): + """Comments by the discussion author are not counted.""" + discussion = self._make_discussion( + "op", + [ + { + "createdAt": "2024-01-02T00:00:00Z", + "author": {"login": "op", "__typename": "User"}, + }, + ], + ) + result = count_comments_per_user(None, discussion=discussion) + self.assertEqual(result, {}) + + def test_ignores_bot_commenter(self): + """Comments from Bot actors are not counted.""" + discussion = self._make_discussion( + "op", + [ + { + "createdAt": "2024-01-02T00:00:00Z", + "author": {"login": "github-actions", "__typename": "Bot"}, + }, + ], + ) + result = count_comments_per_user(None, discussion=discussion) + self.assertEqual(result, {}) + + def test_ignores_user_in_ignore_list(self): + """Comments by explicitly ignored users are not counted.""" + discussion = self._make_discussion( + "op", + [ + { + "createdAt": "2024-01-02T00:00:00Z", + "author": {"login": "spammer", "__typename": "User"}, + }, + ], + ) + result = count_comments_per_user( + None, discussion=discussion, ignore_users=["spammer"] + ) + self.assertEqual(result, {}) + + def test_multiple_commenters(self): + """Multiple distinct commenters each get their own count.""" + discussion = self._make_discussion( + "op", + [ + { + "createdAt": "2024-01-02T00:00:00Z", + "author": {"login": "alice", "__typename": "User"}, + }, + { + "createdAt": "2024-01-03T00:00:00Z", + "author": {"login": "bob", "__typename": "User"}, + }, + { + "createdAt": "2024-01-04T00:00:00Z", + "author": {"login": "alice", "__typename": "User"}, + }, + ], + ) + result = count_comments_per_user(None, discussion=discussion) + self.assertEqual(result, {"alice": 2, "bob": 1}) + + def test_no_comments(self): + """A discussion with an empty comments list returns an empty dict.""" + discussion = self._make_discussion("op", []) + result = count_comments_per_user(None, discussion=discussion) + self.assertEqual(result, {}) + + def test_respects_max_comments_to_eval(self): + """Comments beyond max_comments_to_eval are not evaluated.""" + comments = [ + { + "createdAt": f"2024-01-{i + 2:02d}T00:00:00Z", + "author": {"login": f"mentor{i}", "__typename": "User"}, + } + for i in range(5) + ] + discussion = self._make_discussion("op", comments) + result = count_comments_per_user( + None, discussion=discussion, max_comments_to_eval=2 + ) + self.assertEqual(result, {"mentor0": 1, "mentor1": 1}) + + def test_caps_count_at_heavily_involved(self): + """A single commenter is capped at heavily_involved comments.""" + comments = [ + { + "createdAt": f"2024-01-{i + 2:02d}T00:00:00Z", + "author": {"login": "mentor", "__typename": "User"}, + } + for i in range(5) + ] + discussion = self._make_discussion("op", comments) + result = count_comments_per_user( + None, discussion=discussion, heavily_involved=3 + ) + self.assertEqual(result, {"mentor": 3}) + + def test_null_author_skipped(self): + """A comment with a null author (deleted account) is skipped gracefully.""" + discussion = self._make_discussion( + "op", + [ + { + "createdAt": "2024-01-02T00:00:00Z", + "author": None, + }, + ], + ) + result = count_comments_per_user(None, discussion=discussion) + self.assertEqual(result, {}) + + def test_null_created_at_skipped(self): + """A comment with a null createdAt (pending) is skipped.""" + discussion = self._make_discussion( + "op", + [ + { + "createdAt": None, + "author": {"login": "mentor", "__typename": "User"}, + }, + ], + ) + result = count_comments_per_user(None, discussion=discussion) + self.assertEqual(result, {}) + + def test_null_discussion_author(self): + """A discussion whose author is null (deleted OP) is handled.""" + discussion = self._make_discussion( + "op", + [ + { + "createdAt": "2024-01-02T00:00:00Z", + "author": {"login": "mentor", "__typename": "User"}, + }, + ], + ) + discussion["author"] = None + result = count_comments_per_user(None, discussion=discussion) + self.assertEqual(result, {"mentor": 1})