Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New rule to detect inactive patch authors #2426

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
208 changes: 208 additions & 0 deletions bugbot/rules/inactive_patch_author.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,208 @@
# # This Source Code Form is subject to the terms of the Mozilla Public
# # License, v. 2.0. If a copy of the MPL was not distributed with this file,
# # You can obtain one at http://mozilla.org/MPL/2.0/.

import logging
import re
from typing import Dict, List

from libmozdata.connection import Connection
from libmozdata.phabricator import PhabricatorAPI
from tenacity import retry, stop_after_attempt, wait_exponential

from bugbot import people, utils
from bugbot.bzcleaner import BzCleaner
from bugbot.nag_me import Nag
from bugbot.user_activity import PHAB_CHUNK_SIZE, UserActivity, UserStatus

logging.basicConfig(level=logging.DEBUG)
PHAB_FILE_NAME_PAT = re.compile(r"phabricator-D([0-9]+)-url\.txt")


class InactivePatchAuthors(BzCleaner, Nag):
"""Bugs with patches authored by inactive patch authors"""

def __init__(self):
super(InactivePatchAuthors, self).__init__()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Python 3, you do not need to pass any arguments to super():

Suggested change
super(InactivePatchAuthors, self).__init__()
super().__init__()

self.phab = PhabricatorAPI(utils.get_login_info()["phab_api_key"])
self.user_activity = UserActivity(include_fields=["nick"], phab=self.phab)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the nick field needed here?

self.default_assignees = utils.get_default_assignees()
self.people = people.People.get_instance()
self.no_bugmail = True

def description(self):
return "Bugs with inactive patch authors"

def columns(self):
return ["id", "summary"]

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"]}

inactive_authors = self._get_inactive_patch_authors(list(rev_ids))

for bugid, bug in list(bugs.items()):
inactive_patches = [
{"rev_id": rev_id, "author": inactive_authors[rev_id]}
for rev_id in bug["rev_ids"]
if rev_id in inactive_authors
]

if inactive_patches:
bug["inactive_patches"] = inactive_patches
self.unassign_inactive_author(bugid, bug, inactive_patches)
self.add([bug["assigned_to"], bug["triage_owner"]], bug)
else:
del bugs[bugid]

return bugs

def nag_template(self):
return self.name() + ".html"

def unassign_inactive_author(self, bugid, bug, inactive_patches):
prod = bug["product"]
comp = bug["component"]
default_assignee = self.default_assignees[prod][comp]
autofix = {"assigned_to": default_assignee}

comment = (
"The patch author is inactive on Bugzilla, so the assignee is being reset."
)
autofix["comment"] = {"body": comment}

# Abandon the patches
for patch in inactive_patches:
rev_id = patch["rev_id"]
revision = self.phab.request(
"differential.revision.search",
constraints={"ids": [rev_id]},
)["data"][0]
try:
if revision["fields"]["status"]["value"] in [
"needs-review",
"needs-revision",
"accepted",
"changed-planned",
]:
self.phab.request(
"differential.revision.edit",
objectIdentifier=rev_id,
transactions=[{"type": "abandon", "value": True}],
)
logging.info(f"Abandoned patch {rev_id} for bug {bugid}.")
else:
logging.info(f"Patch {rev_id} for bug {bugid} is already closed.")

except Exception as e:
logging.error(f"Failed to abandon patch {rev_id} for bug {bugid}: {e}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as #2426 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed here: 3a88716


self.autofix_changes[bugid] = autofix

def _get_inactive_patch_authors(self, rev_ids: list) -> Dict[int, dict]:
revisions: List[dict] = []

for _rev_ids in Connection.chunks(rev_ids, PHAB_CHUNK_SIZE):
try:
for revision in self._fetch_revisions(_rev_ids):
author_phid = revision["fields"]["authorPHID"]
created_at = revision["fields"]["dateCreated"]
# if author_phid == "PHID-USER-eltrc7x5oplwzfguutrb":
# continue
revisions.append(
{
"rev_id": revision["id"],
"author_phid": author_phid,
"created_at": created_at,
"status": revision["fields"]["status"]["value"],
}
)
except Exception as e:
logging.error(f"Error fetching revisions: {e}")
continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as #2426 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed here: 9eabe77


user_phids = set()

for revision in revisions:
user_phids.add(revision["author_phid"])

users = self.user_activity.get_phab_users_with_status(
list(user_phids), keep_active=False
)

result: Dict[int, dict] = {}
for revision in revisions:
author_phid = revision["author_phid"]

if author_phid not in users:
continue

author_info = users[author_phid]
if author_info["status"] == UserStatus.INACTIVE:
result[revision["rev_id"]] = {
"name": author_info["name"],
"status": author_info["status"],
"last_active": author_info.get("last_seen_date"),
}

return result

@retry(
wait=wait_exponential(min=4),
stop=stop_after_attempt(5),
)
def _fetch_revisions(self, ids: list):
return self.phab.request(
"differential.revision.search",
constraints={"ids": ids},
)["data"]

def handle_bug(self, bug, data):
rev_ids = [
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

bugid = str(bug["id"])
data[bugid] = {
"rev_ids": rev_ids,
"product": bug["product"],
"component": bug["component"],
"assigned_to": bug["assigned_to"],
"triage_owner": bug["triage_owner"],
}
return bug

def get_bz_params(self, date):
fields = [
"comments.raw_text",
"comments.creator",
"attachments.file_name",
"attachments.content_type",
"attachments.is_obsolete",
"product",
"component",
"assigned_to",
"triage_owner",
]
params = {
"include_fields": fields,
"resolution": "---",
"f1": "attachments.ispatch",
"o1": "equals",
"v1": "1",
}

return params


if __name__ == "__main__":
InactivePatchAuthors().run()
45 changes: 27 additions & 18 deletions bugbot/user_activity.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# License, v. 2.0. If a copy of the MPL was not distributed with this file,
# You can obtain one at http://mozilla.org/MPL/2.0/.

import logging
from datetime import timedelta
from enum import Enum, auto
from typing import Iterable, List, Optional
Expand All @@ -15,6 +16,8 @@
from bugbot import utils
from bugbot.people import People

logging.basicConfig(level=logging.DEBUG)

# The chunk size here should not be more than 100; which is the maximum number of
# items that Phabricator could return in one response.
PHAB_CHUNK_SIZE = 100
Expand Down Expand Up @@ -264,26 +267,32 @@ def get_phab_users_with_status(
# will rely on the calendar from phab.
for _user_phids in Connection.chunks(user_phids, PHAB_CHUNK_SIZE):
for phab_user in self._fetch_phab_users(_user_phids):
user = users[phab_user["phid"]]
phab_status = self._get_status_from_phab_user(phab_user)
if phab_status:
user["status"] = phab_status

elif user["status"] in (
UserStatus.ABSENT,
UserStatus.INACTIVE,
) and self.is_active_on_phab(phab_user["phid"]):
user["status"] = UserStatus.ACTIVE

if not keep_active and user["status"] == UserStatus.ACTIVE:
del users[phab_user["phid"]]
try:
user = users[phab_user["phid"]]
phab_status = self._get_status_from_phab_user(phab_user)
if phab_status:
user["status"] = phab_status

elif user["status"] in (
UserStatus.ABSENT,
UserStatus.INACTIVE,
) and self.is_active_on_phab(phab_user["phid"]):
user["status"] = UserStatus.ACTIVE

if not keep_active and user["status"] == UserStatus.ACTIVE:
del users[phab_user["phid"]]
continue

user["phab_username"] = phab_user["fields"]["username"]
user["unavailable_until"] = phab_user["attachments"][
"availability"
]["until"]
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as #2426 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed here: d1b0350

logging.error(
f"Error fetching inactive patch authors: '{phab_user['phid']}' - {str(e)}"
)
continue

user["phab_username"] = phab_user["fields"]["username"]
user["unavailable_until"] = phab_user["attachments"]["availability"][
"until"
]

return users

def is_active_on_phab(self, user_phid: str) -> bool:
Expand Down
22 changes: 22 additions & 0 deletions templates/inactive_patch_author.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<p>The following {{ plural('bug has', data, pword='bugs have') }} patches with inactive authors:</p>
<table {{ table_attrs }}>
<thead>
<tr>
<th>Bug</th>
<th>Summary</th>
<th>Patches</th>
</tr>
</thead>
<tbody>
{% for i, (bugid, summary) in enumerate(data) -%}
<tr {% if i % 2 == 0 %}bgcolor="#E0E0E0"
{% endif -%}
>
<td {% if bugid in no_manager %}style="background:red;"{% endif %}>
<a href="https://bugzilla.mozilla.org/show_bug.cgi?id={{ bugid }}">{{ bugid }}</a>
</td>
<td>{{ summary | e }}</td>
</tr>
{% endfor -%}
</tbody>
</table>