From 396fad13237a9b8d100f089bd5cb114c52f4eee7 Mon Sep 17 00:00:00 2001 From: Benjamin Mah Date: Wed, 28 Aug 2024 14:40:17 -0400 Subject: [PATCH 1/3] Added util function for `get_bugs()` --- bugbot/inactive_utils.py | 23 +++++++++++++++++++++++ bugbot/rules/inactive_reviewer.py | 21 ++++----------------- 2 files changed, 27 insertions(+), 17 deletions(-) create mode 100644 bugbot/inactive_utils.py diff --git a/bugbot/inactive_utils.py b/bugbot/inactive_utils.py new file mode 100644 index 000000000..eba668b1a --- /dev/null +++ b/bugbot/inactive_utils.py @@ -0,0 +1,23 @@ +from bugbot import utils + + +def process_bugs(bugs, get_revisions_fn, needinfo_func): + rev_ids = {rev_id for bug in bugs.values() for rev_id in bug["rev_ids"]} + revisions = get_revisions_fn(list(rev_ids)) + + for bugid, bug in list(bugs.items()): + inactive_revs = [ + revisions[rev_id] for rev_id in bug["rev_ids"] if rev_id in revisions + ] + if inactive_revs: + bug["revisions"] = inactive_revs + needinfo_user = needinfo_func(bugid, inactive_revs) + bug["needinfo_user"] = needinfo_user + else: + del bugs[bugid] + + # Resolving https://github.com/mozilla/bugbot/issues/1300 should clean this + # including improve the wording in the template (i.e., "See the search query on Bugzilla"). + query_url = utils.get_bz_search_url({"bug_id": ",".join(bugs.keys())}) + + return bugs, query_url diff --git a/bugbot/rules/inactive_reviewer.py b/bugbot/rules/inactive_reviewer.py index d1f10058b..5ee34083e 100644 --- a/bugbot/rules/inactive_reviewer.py +++ b/bugbot/rules/inactive_reviewer.py @@ -15,6 +15,7 @@ from bugbot import utils from bugbot.bzcleaner import BzCleaner from bugbot.history import History +from bugbot.inactive_utils import process_bugs from bugbot.user_activity import PHAB_CHUNK_SIZE, UserActivity, UserStatus PHAB_FILE_NAME_PAT = re.compile(r"phabricator-D([0-9]+)-url\.txt") @@ -48,23 +49,9 @@ def columns(self): def get_bugs(self, date="today", bug_ids=[], chunk_size=None): bugs = super().get_bugs(date, bug_ids, chunk_size) - - rev_ids = {rev_id for bug in bugs.values() for rev_id in bug["rev_ids"]} - revisions = self._get_revisions_with_inactive_reviewers(list(rev_ids)) - - for bugid, bug in list(bugs.items()): - inactive_revs = [ - revisions[rev_id] for rev_id in bug["rev_ids"] if rev_id in revisions - ] - if inactive_revs: - bug["revisions"] = inactive_revs - self._add_needinfo(bugid, inactive_revs) - else: - del bugs[bugid] - - # Resolving https://github.com/mozilla/bugbot/issues/1300 should clean this - # including improve the wording in the template (i.e., "See the search query on Bugzilla"). - self.query_url = utils.get_bz_search_url({"bug_id": ",".join(bugs.keys())}) + bugs, self.query_url = process_bugs( + bugs, self._get_revisions_with_inactive_reviewers, self._add_needinfo + ) return bugs From 92c927eed30cb7a771e679c21b21e2751bc13d93 Mon Sep 17 00:00:00 2001 From: Benjamin Mah Date: Wed, 28 Aug 2024 15:11:03 -0400 Subject: [PATCH 2/3] Added `handle_bug_util()` util function --- bugbot/inactive_utils.py | 43 +++++++++++++++++++++++++++++ bugbot/rules/inactive_reviewer.py | 45 +++---------------------------- 2 files changed, 47 insertions(+), 41 deletions(-) diff --git a/bugbot/inactive_utils.py b/bugbot/inactive_utils.py index eba668b1a..5b55240fe 100644 --- a/bugbot/inactive_utils.py +++ b/bugbot/inactive_utils.py @@ -21,3 +21,46 @@ def process_bugs(bugs, get_revisions_fn, needinfo_func): query_url = utils.get_bz_search_url({"bug_id": ",".join(bugs.keys())}) return bugs, query_url + + +def handle_bug_util(bug, data, PHAB_FILE_NAME_PAT, PHAB_TABLE_PAT, bot): + rev_ids = [ + # To avoid loading the attachment content (which can be very large), + # we extract the revision id from the file name, which is in the + # format of "phabricator-D{revision_id}-url.txt". + # len("phabricator-D") == 13 + # len("-url.txt") == 8 + int(attachment["file_name"][13:-8]) + for attachment in bug["attachments"] + if attachment["content_type"] == "text/x-phabricator-request" + and PHAB_FILE_NAME_PAT.match(attachment["file_name"]) + and not attachment["is_obsolete"] + ] + + if not rev_ids: + return + + # We should not comment about the same patch more than once. + rev_ids_with_ni = set() + for comment in bug["comments"]: + if comment["creator"] == bot and comment["raw_text"].startswith( + "The following patch" + ): + rev_ids_with_ni.update( + int(id) for id in PHAB_TABLE_PAT.findall(comment["raw_text"]) + ) + + if rev_ids_with_ni: + rev_ids = [id for id in rev_ids if id not in rev_ids_with_ni] + + if not rev_ids: + return + + # It will be nicer to show a sorted list of patches + rev_ids.sort() + + bugid = str(bug["id"]) + data[bugid] = { + "rev_ids": rev_ids, + } + return bug diff --git a/bugbot/rules/inactive_reviewer.py b/bugbot/rules/inactive_reviewer.py index 5ee34083e..800c51892 100644 --- a/bugbot/rules/inactive_reviewer.py +++ b/bugbot/rules/inactive_reviewer.py @@ -15,7 +15,7 @@ from bugbot import utils from bugbot.bzcleaner import BzCleaner from bugbot.history import History -from bugbot.inactive_utils import process_bugs +from bugbot.inactive_utils import handle_bug_util, process_bugs from bugbot.user_activity import PHAB_CHUNK_SIZE, UserActivity, UserStatus PHAB_FILE_NAME_PAT = re.compile(r"phabricator-D([0-9]+)-url\.txt") @@ -229,46 +229,9 @@ def _fetch_revisions(self, ids: list): )["data"] def handle_bug(self, bug, data): - rev_ids = [ - # To avoid loading the attachment content (which can be very large), - # we extract the revision id from the file name, which is in the - # format of "phabricator-D{revision_id}-url.txt". - # len("phabricator-D") == 13 - # len("-url.txt") == 8 - int(attachment["file_name"][13:-8]) - for attachment in bug["attachments"] - if attachment["content_type"] == "text/x-phabricator-request" - and PHAB_FILE_NAME_PAT.match(attachment["file_name"]) - and not attachment["is_obsolete"] - ] - - if not rev_ids: - return - - # We should not comment about the same patch more than once. - rev_ids_with_ni = set() - for comment in bug["comments"]: - if comment["creator"] == History.BOT and comment["raw_text"].startswith( - "The following patch" - ): - rev_ids_with_ni.update( - int(id) for id in PHAB_TABLE_PAT.findall(comment["raw_text"]) - ) - - if rev_ids_with_ni: - rev_ids = [id for id in rev_ids if id not in rev_ids_with_ni] - - if not rev_ids: - return - - # It will be nicer to show a sorted list of patches - rev_ids.sort() - - bugid = str(bug["id"]) - data[bugid] = { - "rev_ids": rev_ids, - } - return bug + return handle_bug_util( + bug, data, PHAB_FILE_NAME_PAT, PHAB_TABLE_PAT, History.BOT + ) def get_bz_params(self, date): fields = [ From f3852a6e88633e08ab82b828d2635ff40ccf3382 Mon Sep 17 00:00:00 2001 From: Benjamin Mah Date: Wed, 28 Aug 2024 15:39:40 -0400 Subject: [PATCH 3/3] Added `get_revisions` as a util function --- bugbot/inactive_utils.py | 100 +++++++++++++++++++++++++++++ bugbot/rules/inactive_reviewer.py | 101 +++--------------------------- 2 files changed, 108 insertions(+), 93 deletions(-) diff --git a/bugbot/inactive_utils.py b/bugbot/inactive_utils.py index 5b55240fe..0795c8898 100644 --- a/bugbot/inactive_utils.py +++ b/bugbot/inactive_utils.py @@ -64,3 +64,103 @@ def handle_bug_util(bug, data, PHAB_FILE_NAME_PAT, PHAB_TABLE_PAT, bot): "rev_ids": rev_ids, } return bug + + +def get_revisions( + revs, + fetch_revs_func, + get_phab_users_with_status_func, + status_note_func, + user_status_active, +): + revisions = [] + + for _rev_ids in revs: + for revision in fetch_revs_func(_rev_ids): + if ( + len(revision["attachments"]["reviewers"]["reviewers"]) == 0 + or revision["fields"]["status"]["value"] != "needs-review" + or revision["fields"]["isDraft"] + ): + continue + + reviewers = [ + { + "phid": reviewer["reviewerPHID"], + "is_group": reviewer["reviewerPHID"].startswith("PHID-PROJ"), + "is_blocking": reviewer["isBlocking"], + "is_accepted": reviewer["status"] == "accepted", + "is_resigned": reviewer["status"] == "resigned", + } + for reviewer in revision["attachments"]["reviewers"]["reviewers"] + ] + + # Group reviewers will be consider always active; so if there is + # no other reviewers blocking, we don't need to check further. + if any( + reviewer["is_group"] or reviewer["is_accepted"] + for reviewer in reviewers + ) and not any( + not reviewer["is_accepted"] + for reviewer in reviewers + if reviewer["is_blocking"] + ): + continue + + revisions.append( + { + "rev_id": revision["id"], + "title": revision["fields"]["title"], + "created_at": revision["fields"]["dateCreated"], + "author_phid": revision["fields"]["authorPHID"], + "reviewers": reviewers, + } + ) + + user_phids = set() + for revision in revisions: + user_phids.add(revision["author_phid"]) + for reviewer in revision["reviewers"]: + user_phids.add(reviewer["phid"]) + + users = get_phab_users_with_status_func(list(user_phids), keep_active=True) + + result = {} + for revision in revisions: + # It is not useful to notify an inactive author about an inactive + # reviewer, thus we should exclude revisions with inactive authors. + author_info = users[revision["author_phid"]] + if author_info["status"] != user_status_active: + continue + + revision["author"] = author_info + + inactive_reviewers = [] + for reviewer in revision["reviewers"]: + if reviewer["is_group"]: + continue + + reviewer_info = users[reviewer["phid"]] + if ( + not reviewer["is_resigned"] + and reviewer_info["status"] == user_status_active + ): + continue + + reviewer["info"] = reviewer_info + inactive_reviewers.append(reviewer) + + if len(inactive_reviewers) == len(revision["reviewers"]) or any( + reviewer["is_blocking"] and not reviewer["is_accepted"] + for reviewer in inactive_reviewers + ): + revision["reviewers"] = [ + { + "phab_username": reviewer["info"]["phab_username"], + "status_note": status_note_func(reviewer), + } + for reviewer in inactive_reviewers + ] + result[revision["rev_id"]] = revision + + return result diff --git a/bugbot/rules/inactive_reviewer.py b/bugbot/rules/inactive_reviewer.py index 800c51892..051a63bee 100644 --- a/bugbot/rules/inactive_reviewer.py +++ b/bugbot/rules/inactive_reviewer.py @@ -4,7 +4,7 @@ import re from datetime import datetime -from typing import Dict, List +from typing import Dict from dateutil.relativedelta import relativedelta from libmozdata import utils as lmdutils @@ -15,7 +15,7 @@ from bugbot import utils from bugbot.bzcleaner import BzCleaner from bugbot.history import History -from bugbot.inactive_utils import handle_bug_util, process_bugs +from bugbot.inactive_utils import get_revisions, handle_bug_util, process_bugs from bugbot.user_activity import PHAB_CHUNK_SIZE, UserActivity, UserStatus PHAB_FILE_NAME_PAT = re.compile(r"phabricator-D([0-9]+)-url\.txt") @@ -105,99 +105,14 @@ def _add_needinfo(self, bugid: str, inactive_revs: list) -> None: } def _get_revisions_with_inactive_reviewers(self, rev_ids: list) -> Dict[int, dict]: - revisions: List[dict] = [] - for _rev_ids in Connection.chunks(rev_ids, PHAB_CHUNK_SIZE): - for revision in self._fetch_revisions(_rev_ids): - if ( - len(revision["attachments"]["reviewers"]["reviewers"]) == 0 - or revision["fields"]["status"]["value"] != "needs-review" - or revision["fields"]["isDraft"] - ): - continue - - reviewers = [ - { - "phid": reviewer["reviewerPHID"], - "is_group": reviewer["reviewerPHID"].startswith("PHID-PROJ"), - "is_blocking": reviewer["isBlocking"], - "is_accepted": reviewer["status"] == "accepted", - "is_resigned": reviewer["status"] == "resigned", - } - for reviewer in revision["attachments"]["reviewers"]["reviewers"] - ] - - # Group reviewers will be consider always active; so if there is - # no other reviewers blocking, we don't need to check further. - if any( - reviewer["is_group"] or reviewer["is_accepted"] - for reviewer in reviewers - ) and not any( - not reviewer["is_accepted"] - for reviewer in reviewers - if reviewer["is_blocking"] - ): - continue - - revisions.append( - { - "rev_id": revision["id"], - "title": revision["fields"]["title"], - "created_at": revision["fields"]["dateCreated"], - "author_phid": revision["fields"]["authorPHID"], - "reviewers": reviewers, - } - ) - - user_phids = set() - for revision in revisions: - user_phids.add(revision["author_phid"]) - for reviewer in revision["reviewers"]: - user_phids.add(reviewer["phid"]) - - users = self.user_activity.get_phab_users_with_status( - list(user_phids), keep_active=True + return get_revisions( + Connection.chunks(rev_ids, PHAB_CHUNK_SIZE), + self._fetch_revisions, + self.user_activity.get_phab_users_with_status, + self._get_reviewer_status_note, + UserStatus.ACTIVE, ) - result: Dict[int, dict] = {} - for revision in revisions: - # It is not useful to notify an inactive author about an inactive - # reviewer, thus we should exclude revisions with inactive authors. - author_info = users[revision["author_phid"]] - if author_info["status"] != UserStatus.ACTIVE: - continue - - revision["author"] = author_info - - inactive_reviewers = [] - for reviewer in revision["reviewers"]: - if reviewer["is_group"]: - continue - - reviewer_info = users[reviewer["phid"]] - if ( - not reviewer["is_resigned"] - and reviewer_info["status"] == UserStatus.ACTIVE - ): - continue - - reviewer["info"] = reviewer_info - inactive_reviewers.append(reviewer) - - if len(inactive_reviewers) == len(revision["reviewers"]) or any( - reviewer["is_blocking"] and not reviewer["is_accepted"] - for reviewer in inactive_reviewers - ): - revision["reviewers"] = [ - { - "phab_username": reviewer["info"]["phab_username"], - "status_note": self._get_reviewer_status_note(reviewer), - } - for reviewer in inactive_reviewers - ] - result[revision["rev_id"]] = revision - - return result - @staticmethod def _get_reviewer_status_note(reviewer: dict) -> str: if reviewer["is_resigned"]: