Needinfo the author or reviewer for inactive revisions on Phabricator #2417

benjaminmah · 2024-06-03T19:45:12Z

Resolves #1533.

Intended to:

Needinfo the author if the last action was by the reviewer
Needinfo the reviewer if the last action was by the author
Only needinfo if the revision state is needs-review

Checklist

Type annotations added to new functions
Docs added to functions touched in main classes
Dry-run produced the expected results
The to-be-announced tag added if this is worth announcing

benjaminmah · 2024-06-03T19:45:39Z

Still a WIP!

…ionality

…tches rather than inactive reviewers

benjaminmah · 2024-07-09T19:37:17Z

@suhaibmujahid Since this PR introduces a new rule, would we have to update https://wiki.mozilla.org/BugBot?

marco-c · 2024-07-24T16:56:49Z

@benjaminmah could you dry-run it and share the results?

benjaminmah · 2024-07-24T17:52:10Z

@benjaminmah could you dry-run it and share the results?

Here are the results of the dry-run: dryrun.log

bugbot/rules/inactive_revision.py

marco-c · 2024-08-05T16:50:35Z

bugbot/rules/inactive_revision.py

+        )
+        return response["data"]
+
+    def handle_bug(self, bug, data):


Instead of querying bugs and then finding revisions attached to the bugs, we could search for revisions directly. WDYT @suhaibmujahid?

We will need to query the bugs anyway to ensure we did not comment on them. All the queried bugs have attached revisions. Doing it the other way could be more efficient (not sure), but it will be a bit more complex since BzCleaner is designed to start from bugs.

templates/inactive_revision.html

marco-c · 2024-08-05T16:58:41Z

templates/inactive_revision_needinfo_author.txt

@@ -0,0 +1,3 @@
+:{{ nicknames }}, your patch is still waiting for action. Could you please address the feedback from the reviewer {%- if has_old_patch %} or consider abandoning the patch if it is no longer relevant {%- endif %}?


A problem I see with this is if the reviewer's last action is something like adding a comment saying "I'll get to this soon". I guess we have to accept this will never be entirely precise :)

marco-c · 2024-08-05T16:59:51Z

I checked three random results from the dry-run and they looked OK. @benjaminmah could you double-check three random ones yourself too to make sure they look good? In case of any doubts, feel free to ask me or @suhaibmujahid for our input

benjaminmah · 2024-08-06T22:46:45Z

I checked three random results from the dry-run and they looked OK. @benjaminmah could you double-check three random ones yourself too to make sure they look good? In case of any doubts, feel free to ask me or @suhaibmujahid for our input

Here are the results of the dry-run: results.log

Overall, the randomly selected results seem okay. However, as you mentioned in your other comment, there are a few cases where the reviewer/author leaves a comment without expecting a response (e.g. "Will work on this"). However, not sure if there is an easy way to identify these types of comments.

marco-c · 2024-08-07T15:49:02Z

To be extra cautious, let's add a limit of 1 action per day so we can see the impact.
After Dublin, we can evaluate and remove the limit if all is good.

benjaminmah · 2024-08-07T18:28:06Z

To be extra cautious, let's add a limit of 1 action per day so we can see the impact. After Dublin, we can evaluate and remove the limit if all is good.

I've enforced a limit for max_actions here: 3efbfd1

Let me know if this looks okay!

suhaibmujahid · 2024-08-08T20:30:59Z

bugbot/rules/inactive_revision.py

+# You can obtain one at http://mozilla.org/MPL/2.0/.
+
+import re
+from typing import Dict, List


Let us use dect and list instead.

Done here: 8172791

suhaibmujahid · 2024-08-08T20:31:26Z

bugbot/rules/inactive_revision.py

+                abandon the patch as an option.
+            patch_activity_months: Number of months since the last activity on the patch.
+        """
+        super(InactiveRevision, self).__init__()


Suggested change

super(InactiveRevision, self).__init__()

super().__init__()

All other rules in BugBot include the rule name and self, so I was thinking we could keep it as it is for consistency. Please let me know if you think otherwise and I can go ahead with this change.

suhaibmujahid · 2024-08-08T20:37:10Z

bugbot/rules/inactive_revision.py

+        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


If there is no reviewer, we may still want to notify the author.

suhaibmujahid · 2024-08-08T20:38:58Z

bugbot/rules/inactive_revision.py

+                if (
+                    last_transaction
+                    and last_transaction["dateCreated"] < self.patch_activity_limit
+                ):


It could be more explicit if you flip the condition and use continue.

Good point. Done here: 69385c6.

suhaibmujahid · 2024-08-08T22:56:47Z

bugbot/rules/inactive_revision.py

+            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"])
+                )


This matches comments from the 'inactive_reviewer' rule, but not from this rule. Also, we do not list patch IDs in the needinfo comments, so this is not going to work.

suhaibmujahid · 2024-08-08T23:41:10Z

bugbot/rules/inactive_revision.py

+        )
+        return response["data"]
+
+    def handle_bug(self, bug, data):


We will need to query the bugs anyway to ensure we did not comment on them. All the queried bugs have attached revisions. Doing it the other way could be more efficient (not sure), but it will be a bit more complex since BzCleaner is designed to start from bugs.

suhaibmujahid · 2024-08-08T23:46:18Z

bugbot/rules/inactive_revision.py

There is a substantial amount of duplicated code from the inactive_reviewer rule.

Here are some potential solutions to mitigate the issues:

Using a superclass to host the common logic

Merge the two rules into one

Create some util functions

This applies to #2426 as well.

@marco-c wdyt?

I vote for the third option (create some util functions) or the second (merge the two rules into one).

I agree, I think creating util functions will reduce duplicate code while still keeping the rule separate. Similar to the no_severity rule, the first step could be to needinfo the pending needinfo'er (which could be the reviewer or author) and the second step could be to unblock bugs and find an active reviewer. Of course, these would be separate rules to avoid having complex multi-step rules similar to no_severity. WDYT?

Initial (incomplete) inactive_revision rule commit

dd98111

benjaminmah added 7 commits June 3, 2024 16:07

Updated function to find the author of the last action

b2fc12f

Added functions from inactive_reviewer.py

e7517fc

Added functions from inactive_reviewer.py to maintain similar funct…

960fca7

…ionality

Removed print statement

f3716bb

Initial templates for inactive_revision.py rule

7e17ee3

Added separate needinfo comments depending on who is being needinfo'ed

20210a1

Removed old needinfo template

ab3df86

benjaminmah requested a review from suhaibmujahid July 3, 2024 20:39

benjaminmah marked this pull request as ready for review July 3, 2024 20:41

suhaibmujahid added the to-be-announced Improvements to be announced: send to dev-platform, firefox-dev and add to EE newsletter label Jul 8, 2024

benjaminmah marked this pull request as draft July 9, 2024 14:14

benjaminmah removed the request for review from suhaibmujahid July 9, 2024 14:16

benjaminmah added 2 commits July 9, 2024 15:32

Fixed _get_revisions_with_inactive_action() to focus on inactive pa…

89b50ac

…tches rather than inactive reviewers

Removed initialization of patch_activity_months arg

d7c6689

benjaminmah marked this pull request as ready for review July 9, 2024 19:36

benjaminmah requested a review from suhaibmujahid July 9, 2024 19:37

Edited email template

d736546

benjaminmah requested a review from marco-c July 23, 2024 19:45

marco-c reviewed Aug 5, 2024

View reviewed changes

bugbot/rules/inactive_revision.py Outdated Show resolved Hide resolved

marco-c reviewed Aug 5, 2024

View reviewed changes

bugbot/rules/inactive_revision.py Outdated Show resolved Hide resolved

marco-c reviewed Aug 5, 2024

View reviewed changes

bugbot/rules/inactive_revision.py Outdated Show resolved Hide resolved

marco-c reviewed Aug 5, 2024

View reviewed changes

templates/inactive_revision.html Show resolved Hide resolved

marco-c reviewed Aug 5, 2024

View reviewed changes

benjaminmah added 5 commits August 6, 2024 09:30

Replaced double negation with all

f739f18

Removed returning "other" case when finding last action

9dbb76a

Added filtering out resigned reviewers

1c07ddd

Added column for needinfo_user in email template

1c83ba3

Added column for needinfo_user

e34cd1f

Added max_actions to the inactive_revision rule

3efbfd1

marco-c previously approved these changes Aug 7, 2024

View reviewed changes

suhaibmujahid reviewed Aug 8, 2024

View reviewed changes

Replaced Dict and List with dict and list

8172791

benjaminmah dismissed marco-c’s stale review via 8172791 August 9, 2024 19:11

Reversed condition and added continue

69385c6

benjaminmah mentioned this pull request Aug 28, 2024

[inactive_reviewer] Avoid duplicate functions taken from the inactive_reviewer rule #2471

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Needinfo the author or reviewer for inactive revisions on Phabricator #2417

Needinfo the author or reviewer for inactive revisions on Phabricator #2417

benjaminmah commented Jun 3, 2024 •

edited by suhaibmujahid

Loading

benjaminmah commented Jun 3, 2024

benjaminmah commented Jul 9, 2024

marco-c commented Jul 24, 2024

benjaminmah commented Jul 24, 2024

marco-c Aug 5, 2024

suhaibmujahid Aug 8, 2024

marco-c Aug 5, 2024

marco-c commented Aug 5, 2024

benjaminmah commented Aug 6, 2024

marco-c commented Aug 7, 2024

benjaminmah commented Aug 7, 2024

suhaibmujahid Aug 8, 2024

benjaminmah Aug 9, 2024

suhaibmujahid Aug 8, 2024

benjaminmah Aug 9, 2024

suhaibmujahid Aug 8, 2024

suhaibmujahid Aug 8, 2024

benjaminmah Aug 9, 2024

suhaibmujahid Aug 8, 2024

suhaibmujahid Aug 8, 2024

suhaibmujahid Aug 8, 2024 •

edited

Loading

marco-c Aug 9, 2024

benjaminmah Aug 9, 2024

		@@ -0,0 +1,3 @@
		:{{ nicknames }}, your patch is still waiting for action. Could you please address the feedback from the reviewer {%- if has_old_patch %} or consider abandoning the patch if it is no longer relevant {%- endif %}?

Needinfo the author or reviewer for inactive revisions on Phabricator #2417

Are you sure you want to change the base?

Needinfo the author or reviewer for inactive revisions on Phabricator #2417

Conversation

benjaminmah commented Jun 3, 2024 • edited by suhaibmujahid Loading

Checklist

benjaminmah commented Jun 3, 2024

benjaminmah commented Jul 9, 2024

marco-c commented Jul 24, 2024

benjaminmah commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marco-c commented Aug 5, 2024

benjaminmah commented Aug 6, 2024

marco-c commented Aug 7, 2024

benjaminmah commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suhaibmujahid Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminmah commented Jun 3, 2024 •

edited by suhaibmujahid

Loading

suhaibmujahid Aug 8, 2024 •

edited

Loading