Skip to content

Commit 94003e2

Browse files
committed
[fix] Address CodeRabbit review on stale-PR bot fix
1 parent 3091e79 commit 94003e2

2 files changed

Lines changed: 64 additions & 28 deletions

File tree

.github/actions/bot-autoassign/stale_pr_bot.py

Lines changed: 30 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@ def __init__(self):
1919
self.DAYS_BEFORE_STALE_WARNING = 7
2020
self.DAYS_BEFORE_UNASSIGN = 14
2121
self.DAYS_BEFORE_FINAL_FOLLOWUP = 60
22-
self.bot_login = os.environ.get("BOT_USERNAME", "openwisp-companion") + "[bot]"
22+
bot_username = os.environ.get("BOT_USERNAME", "openwisp-companion")
23+
self.bot_login = (
24+
bot_username if bot_username.endswith("[bot]") else f"{bot_username}[bot]"
25+
)
2326

2427
@staticmethod
2528
def _commit_activity_date_for_author(commit, pr_author):
@@ -164,34 +167,33 @@ def is_waiting_for_maintainer(
164167
def get_last_changes_requested(self, pr, all_reviews=None):
165168
"""Timestamp of the latest CHANGES_REQUESTED that still represents
166169
a human reviewer's current stance, or ``None``.
170+
171+
Errors propagate so the caller can distinguish "no active block"
172+
from "couldn't determine" and skip the PR.
167173
"""
168-
try:
169-
if all_reviews is None:
170-
all_reviews = list(pr.get_reviews())
171-
# Bot reviews are advisory; COMMENTED does not change stance.
172-
latest_per_reviewer = {}
173-
for review in all_reviews:
174-
if (
175-
not review.user
176-
or not review.submitted_at
177-
or review.user.type == "Bot"
178-
or review.state == "COMMENTED"
179-
):
180-
continue
181-
current = latest_per_reviewer.get(review.user.login)
182-
if current is None or review.submitted_at > current.submitted_at:
183-
latest_per_reviewer[review.user.login] = review
184-
return max(
185-
(
186-
review.submitted_at
187-
for review in latest_per_reviewer.values()
188-
if review.state == "CHANGES_REQUESTED"
189-
),
190-
default=None,
191-
)
192-
except Exception as e:
193-
print(f"Error getting reviews for PR #{pr.number}: {e}")
194-
return None
174+
if all_reviews is None:
175+
all_reviews = list(pr.get_reviews())
176+
# Bot reviews are advisory; COMMENTED does not change stance.
177+
latest_per_reviewer = {}
178+
for review in all_reviews:
179+
if (
180+
not review.user
181+
or not review.submitted_at
182+
or review.user.type == "Bot"
183+
or review.state == "COMMENTED"
184+
):
185+
continue
186+
current = latest_per_reviewer.get(review.user.login)
187+
if current is None or review.submitted_at > current.submitted_at:
188+
latest_per_reviewer[review.user.login] = review
189+
return max(
190+
(
191+
review.submitted_at
192+
for review in latest_per_reviewer.values()
193+
if review.state == "CHANGES_REQUESTED"
194+
),
195+
default=None,
196+
)
195197

196198
def has_bot_comment(self, pr, comment_type, after_date=None, issue_comments=None):
197199
"""Check if this bot has already posted a comment with the given

.github/actions/bot-autoassign/tests/test_stale_pr_bot.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -711,6 +711,40 @@ def test_pr_first_processed_past_60_days_marks_stale_only(
711711
mock_pr.add_to_labels.assert_called_once_with("stale")
712712
mock_pr.edit.assert_not_called()
713713

714+
@patch("stale_pr_bot.datetime")
715+
def test_final_followup_fires_after_prior_stale_run(self, mock_datetime, bot_env):
716+
mock_datetime.now.return_value = datetime(2024, 5, 10, tzinfo=timezone.utc)
717+
mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw)
718+
bot = StalePRBot()
719+
mock_pr = Mock()
720+
mock_pr.body = ""
721+
mock_pr.number = 7
722+
mock_pr.user.login = "contributor"
723+
cr_review = Mock()
724+
cr_review.state = "CHANGES_REQUESTED"
725+
cr_review.submitted_at = datetime(2024, 2, 1, tzinfo=timezone.utc)
726+
cr_review.user.login = "maintainer"
727+
cr_review.user.type = "User"
728+
mock_pr.get_reviews.return_value = [cr_review]
729+
mock_pr.get_commits.return_value = []
730+
mock_pr.get_review_comments.return_value = []
731+
stale_label = Mock()
732+
stale_label.name = "stale"
733+
mock_pr.get_labels.return_value = [stale_label]
734+
prior_stale = Mock()
735+
prior_stale.user.login = bot.bot_login
736+
prior_stale.body = "<!-- bot:stale --> previous run"
737+
prior_stale.created_at = datetime(2024, 5, 1, tzinfo=timezone.utc)
738+
mock_pr.get_issue_comments.return_value = [prior_stale]
739+
bot_env["repo"].get_pulls.return_value = [mock_pr]
740+
bot.process_stale_prs()
741+
bodies = [c[0][0] for c in mock_pr.create_issue_comment.call_args_list]
742+
assert any("<!-- bot:final_followup -->" in b for b in bodies)
743+
assert not any(
744+
"<!-- bot:stale -->" in b and "previous run" not in b for b in bodies
745+
)
746+
mock_pr.edit.assert_not_called()
747+
714748
@patch("stale_pr_bot.datetime")
715749
def test_clears_stale_label_when_contributor_responds(self, mock_datetime, bot_env):
716750
mock_datetime.now.return_value = datetime(2024, 2, 1, tzinfo=timezone.utc)

0 commit comments

Comments
 (0)