From 22f06ecee566ac4b43de2cf2b16831c6406374e3 Mon Sep 17 00:00:00 2001 From: prathmeshkulkarni-coder Date: Fri, 10 Apr 2026 22:05:12 +0530 Subject: [PATCH 1/2] [fix] Automate branch selection for porting changelog #646 Implemented automated branch detection to avoid choosing between master and main when only one exists. Fixes #646 --- openwisp_utils/releaser/release.py | 22 +++- openwisp_utils/releaser/tests/test_release.py | 124 +++++++++++++++++- openwisp_utils/releaser/tests/test_utils.py | 18 +++ openwisp_utils/releaser/utils.py | 11 ++ 4 files changed, 167 insertions(+), 8 deletions(-) diff --git a/openwisp_utils/releaser/release.py b/openwisp_utils/releaser/release.py index 6e32079e9..82d1973c2 100644 --- a/openwisp_utils/releaser/release.py +++ b/openwisp_utils/releaser/release.py @@ -20,6 +20,7 @@ from openwisp_utils.releaser.utils import ( SkipSignal, adjust_markdown_headings, + branch_exists, demote_markdown_headings, format_file_with_docstrfmt, get_current_branch, @@ -176,10 +177,23 @@ def port_changelog_to_main(gh, config, version, changelog_body, original_branch) full_block_to_port = f"{version_header}\n{underline}\n\n{changelog_body}" try: - main_branch = questionary.select( - "Which branch should the changelog be ported to?", - choices=MAIN_BRANCHES, - ).ask() + master_exists = branch_exists("master") + main_exists = branch_exists("main") + if master_exists and main_exists: + main_branch = questionary.select( + "Which branch should the changelog be ported to?", + choices=MAIN_BRANCHES, + ).ask() + elif master_exists: + main_branch = "master" + elif main_exists: + main_branch = "main" + else: + print( + "Neither 'master' nor 'main' branches were found locally. " + "Skipping changelog porting." + ) + return if not main_branch: print("Porting cancelled.") diff --git a/openwisp_utils/releaser/tests/test_release.py b/openwisp_utils/releaser/tests/test_release.py index 1f716de45..ffb8c55f9 100644 --- a/openwisp_utils/releaser/tests/test_release.py +++ b/openwisp_utils/releaser/tests/test_release.py @@ -155,13 +155,20 @@ def test_main_flow_pr_merge_wait(mock_all): @patch("openwisp_utils.releaser.release.update_changelog_file") @patch("openwisp_utils.releaser.release.format_file_with_docstrfmt") @patch("openwisp_utils.releaser.release.subprocess.run") +@patch("openwisp_utils.releaser.release.branch_exists") @patch("openwisp_utils.releaser.release.questionary") def test_port_changelog_to_main_flow( - mock_questionary, mock_subprocess, mock_format_file, mock_update_changelog + mock_questionary, + mock_branch_exists, + mock_subprocess, + mock_format_file, + mock_update_changelog, ): """Tests the changelog porting process for both RST and MD files, and the cancellation path.""" mock_gh = MagicMock() mock_config_rst = {"changelog_path": "CHANGES.rst"} + # Both branches exist: user is asked + mock_branch_exists.return_value = True mock_questionary.select.return_value.ask.return_value = "main" port_changelog_to_main(mock_gh, mock_config_rst, "1.1.1", "- fix", "1.1.x") mock_gh.create_pr.assert_called_once() @@ -169,12 +176,115 @@ def test_port_changelog_to_main_flow( mock_gh.reset_mock() - # Test Cancellation path + # Test Cancellation path (when both branches exist and user cancels) mock_questionary.select.return_value.ask.return_value = None port_changelog_to_main(mock_gh, mock_config_rst, "1.1.1", "- fix", "1.1.x") mock_gh.create_pr.assert_not_called() +@patch("openwisp_utils.releaser.release.update_changelog_file") +@patch("openwisp_utils.releaser.release.format_file_with_docstrfmt") +@patch("openwisp_utils.releaser.release.subprocess.run") +@patch("openwisp_utils.releaser.release.branch_exists") +def test_port_changelog_only_master_exists( + mock_branch_exists, mock_subprocess, mock_format_file, mock_update_changelog +): + """`master` is auto-selected when `main` does not exist locally.""" + mock_gh = MagicMock() + mock_config = {"changelog_path": "CHANGES.rst"} + # Simulate: main=False, master=True + mock_branch_exists.side_effect = lambda name: name == "master" + port_changelog_to_main(mock_gh, mock_config, "1.1.1", "- fix", "1.1.x") + mock_gh.create_pr.assert_called_once() + # Verify PR was opened against master + assert mock_gh.create_pr.call_args[0][1] == "master" + + +@patch("openwisp_utils.releaser.release.update_changelog_file") +@patch("openwisp_utils.releaser.release.format_file_with_docstrfmt") +@patch("openwisp_utils.releaser.release.subprocess.run") +@patch("openwisp_utils.releaser.release.branch_exists") +def test_port_changelog_only_main_exists( + mock_branch_exists, mock_subprocess, mock_format_file, mock_update_changelog +): + """`main` is auto-selected when it exists and `master` does not.""" + mock_gh = MagicMock() + mock_config = {"changelog_path": "CHANGES.rst"} + # Simulate: main=True, master=False + mock_branch_exists.side_effect = lambda name: name == "main" + port_changelog_to_main(mock_gh, mock_config, "1.1.1", "- fix", "1.1.x") + mock_gh.create_pr.assert_called_once() + # Verify PR was opened against main + assert mock_gh.create_pr.call_args[0][1] == "main" + + +@patch("openwisp_utils.releaser.release.update_changelog_file") +@patch("openwisp_utils.releaser.release.format_file_with_docstrfmt") +@patch("openwisp_utils.releaser.release.subprocess.run") +@patch("openwisp_utils.releaser.release.branch_exists") +@patch("openwisp_utils.releaser.release.questionary") +def test_port_changelog_both_branches_prompts_user( + mock_questionary, + mock_branch_exists, + mock_subprocess, + mock_format_file, + mock_update_changelog, +): + """User is prompted to choose when both `main` and `master` exist.""" + mock_gh = MagicMock() + mock_config = {"changelog_path": "CHANGES.rst"} + # Both branches exist + mock_branch_exists.return_value = True + mock_questionary.select.return_value.ask.return_value = "master" + port_changelog_to_main(mock_gh, mock_config, "1.1.1", "- fix", "1.1.x") + mock_questionary.select.assert_called_once() + mock_gh.create_pr.assert_called_once() + assert mock_gh.create_pr.call_args[0][1] == "master" + + +@patch("openwisp_utils.releaser.release.update_changelog_file") +@patch("openwisp_utils.releaser.release.format_file_with_docstrfmt") +@patch("openwisp_utils.releaser.release.subprocess.run") +@patch("openwisp_utils.releaser.release.branch_exists") +def test_port_changelog_neither_branch_exists( + mock_branch_exists, mock_subprocess, mock_format_file, mock_update_changelog +): + """Porting is skipped with a message if neither branch exists.""" + mock_gh = MagicMock() + mock_config = {"changelog_path": "CHANGES.rst"} + # Neither exists + mock_branch_exists.return_value = False + port_changelog_to_main(mock_gh, mock_config, "1.1.1", "- fix", "1.1.x") + # Verify no PR was created + mock_gh.create_pr.assert_not_called() + # Verify no file update was attempted + mock_update_changelog.assert_not_called() + + +@patch("openwisp_utils.releaser.release.update_changelog_file") +@patch("openwisp_utils.releaser.release.format_file_with_docstrfmt") +@patch("openwisp_utils.releaser.release.subprocess.run") +@patch("openwisp_utils.releaser.release.branch_exists") +@patch("openwisp_utils.releaser.release.questionary") +def test_port_changelog_cancelled( + mock_questionary, + mock_branch_exists, + mock_subprocess, + mock_format_file, + mock_update_changelog, +): + """Porting is cancelled if user doesn't select a branch.""" + mock_gh = MagicMock() + mock_config = {"changelog_path": "CHANGES.rst"} + # Both exist to trigger prompt + mock_branch_exists.return_value = True + # User cancels (Ctrl+C or Esc) + mock_questionary.select.return_value.ask.return_value = None + port_changelog_to_main(mock_gh, mock_config, "1.1.1", "- fix", "1.1.x") + # Verify no PR was created + mock_gh.create_pr.assert_not_called() + + def test_main_bugfix_flow_with_porting(mock_all, mocker): """Tests the main release flow for a bugfix, including accepting the changelog port.""" mock_all["_git_command_map"][ @@ -239,11 +349,15 @@ def test_main_flow_skip_release_creation(mock_all): ) +@patch("openwisp_utils.releaser.release.branch_exists") @patch("openwisp_utils.releaser.release.subprocess.run") -def test_port_changelog_to_main_flow_markdown(mock_subprocess, mock_all): +def test_port_changelog_to_main_flow_markdown( + mock_subprocess, mock_branch_exists, mock_all +): """Tests the changelog porting process for a Markdown file.""" mock_gh = MagicMock() mock_config_md = {"changelog_path": "CHANGES.md"} + mock_branch_exists.return_value = True mock_all["questionary_select"].return_value.ask.return_value = "main" with patch("openwisp_utils.releaser.release.update_changelog_file") as mock_update: @@ -253,12 +367,14 @@ def test_port_changelog_to_main_flow_markdown(mock_subprocess, mock_all): assert "## Version 1.1.1" in called_with_content +@patch("openwisp_utils.releaser.release.branch_exists") @patch("openwisp_utils.releaser.release.subprocess.run") -def test_port_changelog_skip_pr_creation(mock_subprocess, mock_all): +def test_port_changelog_skip_pr_creation(mock_subprocess, mock_branch_exists, mock_all): """Tests skipping PR creation during changelog porting.""" mock_gh = MagicMock() mock_gh.create_pr.side_effect = SkipSignal mock_config = {"changelog_path": "CHANGES.rst"} + mock_branch_exists.return_value = True mock_all["questionary_select"].return_value.ask.return_value = "main" with patch("openwisp_utils.releaser.release.update_changelog_file"): diff --git a/openwisp_utils/releaser/tests/test_utils.py b/openwisp_utils/releaser/tests/test_utils.py index 07672aadc..468aaf525 100644 --- a/openwisp_utils/releaser/tests/test_utils.py +++ b/openwisp_utils/releaser/tests/test_utils.py @@ -10,6 +10,7 @@ ) from openwisp_utils.releaser.utils import ( SkipSignal, + branch_exists, format_file_with_docstrfmt, retryable_request, ) @@ -306,3 +307,20 @@ def test_retryable_request_retries_then_aborts( assert mock_request.call_count == 2 assert mock_print.call_count == 3 mock_sleep.assert_called_once_with(1) + + +@patch("openwisp_utils.releaser.utils.subprocess.run") +def test_branch_exists_success(mock_run): + """branch_exists returns True when git exit code is 0.""" + mock_run.return_value = MagicMock(returncode=0) + assert branch_exists("master") is True + # Ensure it's looking for refs/heads/master + mock_run.assert_called_once() + assert "refs/heads/master" in mock_run.call_args[0][0] + + +@patch("openwisp_utils.releaser.utils.subprocess.run") +def test_branch_exists_failure(mock_run): + """branch_exists returns False when git exit code is non-zero.""" + mock_run.return_value = MagicMock(returncode=1) + assert branch_exists("non-existent") is False diff --git a/openwisp_utils/releaser/utils.py b/openwisp_utils/releaser/utils.py index b37a034d0..8eb14cb85 100644 --- a/openwisp_utils/releaser/utils.py +++ b/openwisp_utils/releaser/utils.py @@ -85,6 +85,17 @@ def get_current_branch(): return result.stdout.strip() +def branch_exists(branch_name): + """Check if a Git branch exists locally.""" + result = subprocess.run( + ["git", "show-ref", "--verify", "--quiet", f"refs/heads/{branch_name}"], + capture_output=True, + text=True, + encoding="utf-8", + ) + return result.returncode == 0 + + def rst_to_markdown(text): """Convert reStructuredText to Markdown using pypandoc.""" escaped_text = re.sub(r"(? Date: Tue, 7 Jul 2026 12:57:35 +0530 Subject: [PATCH 2/2] [feature] Flag invalid pull requests that do not reference validated issues #709 - Add pull_request_target edited/ready_for_review triggers - Implement regex issue references extraction helper - Add GraphQL project v2 board assignment check - Validate that PRs from external contributors link to open, labeled, and validated organization issues - Apply 'invalid' label and post warning comment if invalid - Automatically close invalid PRs after 24 hours in the stale PR workflow - Update documentation and add unit tests Closes #709 --- .github/actions/bot-autoassign/base.py | 151 ++++++++++++++ .../bot-autoassign/issue_assignment_bot.py | 45 ++++- .../actions/bot-autoassign/stale_pr_bot.py | 81 +++++--- .../tests/test_issue_assignment_bot.py | 190 +++++++++++++++++- .../bot-autoassign/tests/test_stale_pr_bot.py | 83 ++++++++ .github/actions/bot-autoassign/utils.py | 38 ++++ .../bot-autoassign-pr-issue-link.yml | 2 +- docs/developer/reusable-github-utils.rst | 8 +- 8 files changed, 570 insertions(+), 28 deletions(-) diff --git a/.github/actions/bot-autoassign/base.py b/.github/actions/bot-autoassign/base.py index 08e050227..a0042b356 100644 --- a/.github/actions/bot-autoassign/base.py +++ b/.github/actions/bot-autoassign/base.py @@ -25,3 +25,154 @@ def __init__(self): def load_event_payload(self, event_payload): self.event_payload = event_payload + + def get_issue_projects(self, owner, repo_name, issue_number): + query = """ + query($owner: String!, $repo: String!, $issueNumber: Int!) { + repository(owner: $owner, name: $repo) { + issue(number: $issueNumber) { + projectItems(first: 10) { + nodes { + project { + title + } + } + } + } + } + } + """ + variables = { + "owner": owner, + "repo": repo_name, + "issueNumber": issue_number + } + result = self.github.raw_graphql(query, variables) + if "errors" in result: + raise Exception(f"GraphQL errors: {result['errors']}") + + repo_data = result.get("data", {}).get("repository", {}) or {} + issue_data = repo_data.get("issue", {}) or {} + project_items = issue_data.get("projectItems", {}) or {} + nodes = project_items.get("nodes", []) or [] + + return [ + node["project"]["title"] + for node in nodes + if node and node.get("project") and node["project"].get("title") + ] + + def validate_pr_issues(self, pr): + """Validate if a pull request is from an exempt user or references a validated issue.""" + if not self.github or not self.repository_name: + print("GitHub client or repository name not initialized") + return False + + pr_author = pr.user.login if pr.user and isinstance(getattr(pr.user, "login", None), str) else "" + + # Check if the author is in the exclude list + exclude_authors_env = os.environ.get("EXCLUDE_PR_AUTHORS", "dependabot[bot]") + excluded_authors = [auth.strip() for auth in exclude_authors_env.split(",") if auth.strip()] + if pr_author in excluded_authors: + print(f"Author {pr_author} is in the exclude list. Proceeding.") + return True + + # Check if the author is a maintainer/collaborator + author_association = getattr(pr, "author_association", None) + if isinstance(author_association, str) and author_association in ["OWNER", "MEMBER", "COLLABORATOR"]: + print(f"Author {pr_author} is exempt due to association: {author_association}. Proceeding.") + return True + + # For external contributors, extract linked issues + from utils import extract_all_linked_issues + pr_body = pr.body if isinstance(pr.body, str) else "" + linked_issues = extract_all_linked_issues(pr_body, self.repository_name) + if not linked_issues: + print("No linked issues found in PR body for external contributor.") + return False + + current_org = self.repository_name.split("/")[0].lower() + required_projects = [ + "OpenWISP Contributor's Board", + "OpenWISP Priorities for next releases" + ] + + for owner, repo_name, issue_number in linked_issues: + # 1. Check if the issue belongs to the same organization + if owner.lower() != current_org: + print(f"Issue {owner}/{repo_name}#{issue_number} does not belong to organization {current_org}, skipping validation.") + continue + + try: + from github import GithubException + target_repo = self.github.get_repo(f"{owner}/{repo_name}") + issue = target_repo.get_issue(issue_number) + except Exception as e: + if isinstance(e, GithubException) and e.status == 404: + print(f"Issue {owner}/{repo_name}#{issue_number} not found, skipping validation.") + continue + else: + print(f"Error fetching issue {owner}/{repo_name}#{issue_number}: {e}") + raise + + # 2. Check if the issue is a PR + if issue.pull_request: + print(f"Reference {owner}/{repo_name}#{issue_number} is a pull request, skipping validation.") + continue + + # 3. Check if the issue is open + if issue.state != "open": + print(f"Issue {owner}/{repo_name}#{issue_number} is not open, skipping validation.") + continue + + # 4. Check if the issue has at least one label, and none are invalid/wontfix + issue_labels = [label.name.lower() for label in issue.labels] + valid_labels = [l for l in issue_labels if l not in ["invalid", "wontfix"]] + if not valid_labels: + print(f"Issue {owner}/{repo_name}#{issue_number} has no valid labels, skipping validation.") + continue + if any(l in ["invalid", "wontfix"] for l in issue_labels): + print(f"Issue {owner}/{repo_name}#{issue_number} contains invalid/wontfix label, skipping validation.") + continue + + # 5. Check if the issue is assigned to one of the required projects + try: + projects = self.get_issue_projects(owner, repo_name, issue_number) + except Exception as e: + print(f"Error fetching projects for issue {owner}/{repo_name}#{issue_number}: {e}") + raise + + has_valid_project = any(project in required_projects for project in projects) + if has_valid_project: + print(f"Issue {owner}/{repo_name}#{issue_number} is validated. PR is valid.") + return True + else: + print(f"Issue {owner}/{repo_name}#{issue_number} is not assigned to any required project, skipping validation.") + + return False + + def has_bot_comment(self, pr, comment_type, after_date=None, issue_comments=None): + """Check if PR already has a specific type of bot comment. + + Uses HTML markers. If ``after_date`` is provided, + only considers comments posted after that date. + """ + try: + if issue_comments is None: + issue_comments = list(pr.get_issue_comments()) + marker = f"" + for comment in issue_comments: + if ( + comment.user + and comment.user.type == "Bot" + and marker in comment.body + ): + if after_date and comment.created_at <= after_date: + continue + return True + return False + except Exception as e: + print("Error checking bot comments" f" for PR #{pr.number}: {e}") + return False + + diff --git a/.github/actions/bot-autoassign/issue_assignment_bot.py b/.github/actions/bot-autoassign/issue_assignment_bot.py index 740537cd7..9f064633f 100644 --- a/.github/actions/bot-autoassign/issue_assignment_bot.py +++ b/.github/actions/bot-autoassign/issue_assignment_bot.py @@ -305,14 +305,55 @@ def handle_pull_request(self): return False if action in ["opened", "reopened"]: self.auto_assign_issues_from_pr(pr_number, pr_author, pr_body) - # We consider the event handled even if no issues were linked - return True elif action == "closed": if pr.get("merged", False): print(f"PR #{pr_number} was merged, keeping issue assignments") else: self.unassign_issues_from_pr(pr_body, pr_author) return True + + if action in ["opened", "reopened", "edited", "ready_for_review"]: + pr_obj = self.repo.get_pull(pr_number) + is_valid = self.validate_pr_issues(pr_obj) + labels = [] + if hasattr(pr_obj, "labels") and not hasattr(pr_obj.labels, "assert_called"): + try: + labels = [label.name for label in pr_obj.labels] + except TypeError: + pass + + if is_valid: + if "invalid" in labels: + pr_obj.remove_from_labels("invalid") + print(f"Removed 'invalid' label from PR #{pr_number}") + else: + if "invalid" not in labels: + pr_obj.add_to_labels("invalid") + print(f"Added 'invalid' label to PR #{pr_number}") + + if not self.has_bot_comment(pr_obj, "invalid_unvalidated_issue"): + comment_body = ( + "\n\n" + f"Hi @{pr_author},\n\n" + "Thank you for your interest in contributing to OpenWISP.\n\n" + "This pull request has been flagged because external contributors must target an " + "issue validated by maintainers before requesting review.\n\n" + "Please link this pull request to a validated issue by adding " + "`Fixes #ISSUE_NUMBER`, `Closes #ISSUE_NUMBER`, or `Related to #ISSUE_NUMBER` to the pull request description. " + "The issue may be in this repository or another OpenWISP repository.\n\n" + "If there is no validated issue yet, please open one first and wait for " + "maintainer validation before continuing with this pull request.\n\n" + "An issue is considered validated when it is open, has an appropriate label other than " + "`invalid` or `wontfix`, and is assigned to one of the OpenWISP contributor project boards " + "mentioned in the contributing guidelines.\n\n" + "Please see the OpenWISP policy on unsolicited and AI-assisted contributions:\n" + "https://openwisp.io/docs/dev/general/code-of-conduct.html\n\n" + "If this is not resolved within 24 hours, this pull request will be closed automatically. " + "Thank you for your understanding." + ) + pr_obj.create_issue_comment(comment_body) + print(f"Posted unvalidated issue warning comment on PR #{pr_number}") + return True print(f"PR action '{action}' not handled") return True except Exception as e: diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index dfecc7fd0..e1685f20d 100644 --- a/.github/actions/bot-autoassign/stale_pr_bot.py +++ b/.github/actions/bot-autoassign/stale_pr_bot.py @@ -104,29 +104,6 @@ def get_last_changes_requested(self, pr, all_reviews=None): print("Error getting reviews" f" for PR #{pr.number}: {e}") return None - def has_bot_comment(self, pr, comment_type, after_date=None, issue_comments=None): - """Check if PR already has a specific type of bot comment. - - Uses HTML markers. If ``after_date`` is provided, - only considers comments posted after that date. - """ - try: - if issue_comments is None: - issue_comments = list(pr.get_issue_comments()) - marker = f"" - for comment in issue_comments: - if ( - comment.user - and comment.user.type == "Bot" - and marker in comment.body - ): - if after_date and comment.created_at <= after_date: - continue - return True - return False - except Exception as e: - print("Error checking bot comments" f" for PR #{pr.number}: {e}") - return False def unassign_linked_issues(self, pr): try: @@ -329,6 +306,64 @@ def process_stale_prs(self): for pr in open_prs: pr_count += 1 try: + pr_labels = [label.name for label in pr.labels] + if "invalid" in pr_labels: + if self.validate_pr_issues(pr): + pr.remove_from_labels("invalid") + print(f"PR #{pr.number} is now valid. Removed 'invalid' label.") + else: + warning_comment = None + comments = list(pr.get_issue_comments()) + for comment in comments: + if ( + comment.user + and comment.user.type == "Bot" + and "" in comment.body + ): + warning_comment = comment + break + if warning_comment: + from datetime import datetime, timezone + now = datetime.now(timezone.utc) + if (now - warning_comment.created_at).total_seconds() >= 24 * 3600: + close_message = ( + "\n\n" + "This pull request has been automatically closed because it has been flagged as " + "invalid (not referencing a validated issue) for more than 24 hours." + ) + try: + pr.create_issue_comment(close_message) + except Exception as comment_error: + print(f"Warning: Could not post close comment on PR #{pr.number}: {comment_error}") + pr.edit(state="closed") + print(f"Closed PR #{pr.number} automatically after 24 hours.") + processed_count += 1 + else: + pr_author = pr.user.login if pr.user else "" + comment_body = ( + "\n\n" + f"Hi @{pr_author},\n\n" + "Thank you for your interest in contributing to OpenWISP.\n\n" + "This pull request has been flagged because external contributors must target an " + "issue validated by maintainers before requesting review.\n\n" + "Please link this pull request to a validated issue by adding " + "`Fixes #ISSUE_NUMBER`, `Closes #ISSUE_NUMBER`, or `Related to #ISSUE_NUMBER` to the pull request description. " + "The issue may be in this repository or another OpenWISP repository.\n\n" + "If there is no validated issue yet, please open one first and wait for " + "maintainer validation before continuing with this pull request.\n\n" + "An issue is considered validated when it is open, has an appropriate label other than " + "`invalid` or `wontfix`, and is assigned to one of the OpenWISP contributor project boards " + "mentioned in the contributing guidelines.\n\n" + "Please see the OpenWISP policy on unsolicited and AI-assisted contributions:\n" + "https://openwisp.io/docs/dev/general/code-of-conduct.html\n\n" + "If this is not resolved within 24 hours, this pull request will be closed automatically. " + "Thank you for your understanding." + ) + pr.create_issue_comment(comment_body) + print(f"Posted missing warning comment on PR #{pr.number}") + processed_count += 1 + continue + all_reviews = list(pr.get_reviews()) last_changes_requested = self.get_last_changes_requested( pr, all_reviews diff --git a/.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py b/.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py index 16e226f18..ac7ce3b14 100644 --- a/.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py +++ b/.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py @@ -448,9 +448,197 @@ def test_unsupported_event(self, bot_env): bot = IssueAssignmentBot() bot.event_name = "push" assert bot.run() - def test_no_github_client(self, bot_env): bot = IssueAssignmentBot() bot.github = None bot.repo = None assert not bot.run() + + +class TestPRValidation: + def test_get_issue_projects_success(self, bot_env): + bot = IssueAssignmentBot() + bot.github.raw_graphql.return_value = { + "data": { + "repository": { + "issue": { + "projectItems": { + "nodes": [ + {"project": {"title": "OpenWISP Contributor's Board"}}, + {"project": {"title": "Some Other Board"}}, + ] + } + } + } + } + } + projects = bot.get_issue_projects("openwisp", "openwisp-utils", 123) + assert projects == ["OpenWISP Contributor's Board", "Some Other Board"] + bot.github.raw_graphql.assert_called_once() + + def test_get_issue_projects_graphql_error(self, bot_env): + bot = IssueAssignmentBot() + bot.github.raw_graphql.return_value = { + "errors": [{"message": "Some error"}] + } + with pytest.raises(Exception, match="GraphQL errors:"): + bot.get_issue_projects("openwisp", "openwisp-utils", 123) + + def test_validate_pr_issues_excluded_author(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "dependabot[bot]" + assert bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_exempt_association(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "some-member" + mock_pr.author_association = "MEMBER" + assert bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_no_issues(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "No references here" + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_cross_org_issue(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes otherorg/repo#12" + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_is_pr(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes #12" + + mock_issue = Mock() + mock_issue.pull_request = {"url": "..."} + bot_env["repo"].get_issue.return_value = mock_issue + + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_closed(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes #12" + + mock_issue = Mock() + mock_issue.pull_request = None + mock_issue.state = "closed" + bot_env["repo"].get_issue.return_value = mock_issue + + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_invalid_labels(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes #12" + + # Test case 1: No labels + mock_issue = Mock() + mock_issue.pull_request = None + mock_issue.state = "open" + mock_issue.labels = [] + bot_env["repo"].get_issue.return_value = mock_issue + assert not bot.validate_pr_issues(mock_pr) + + # Test case 2: Has wontfix/invalid labels + label_wontfix = Mock() + label_wontfix.name = "wontfix" + mock_issue.labels = [label_wontfix] + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_not_in_project(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes #12" + + label_bug = Mock() + label_bug.name = "bug" + mock_issue = Mock() + mock_issue.pull_request = None + mock_issue.state = "open" + mock_issue.labels = [label_bug] + bot_env["repo"].get_issue.return_value = mock_issue + + with patch.object(bot, "get_issue_projects", return_value=["Some Other Project"]): + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_fully_valid(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes #12" + + label_bug = Mock() + label_bug.name = "bug" + mock_issue = Mock() + mock_issue.pull_request = None + mock_issue.state = "open" + mock_issue.labels = [label_bug] + bot_env["repo"].get_issue.return_value = mock_issue + + with patch.object(bot, "get_issue_projects", return_value=["OpenWISP Contributor's Board"]): + assert bot.validate_pr_issues(mock_pr) + + def test_handle_pull_request_invalid_label_and_comment(self, bot_env): + bot = IssueAssignmentBot() + bot.event_name = "pull_request_target" + bot.load_event_payload({ + "action": "opened", + "pull_request": { + "number": 100, + "user": {"login": "testuser"}, + "body": "Fixes #123", + } + }) + + mock_pr_obj = Mock() + mock_pr_obj.labels = [] + bot_env["repo"].get_pull.return_value = mock_pr_obj + + with patch.object(bot, "validate_pr_issues", return_value=False), \ + patch.object(bot, "has_bot_comment", return_value=False): + assert bot.handle_pull_request() + mock_pr_obj.add_to_labels.assert_called_once_with("invalid") + mock_pr_obj.create_issue_comment.assert_called_once() + assert "invalid_unvalidated_issue" in mock_pr_obj.create_issue_comment.call_args[0][0].lower() + + def test_handle_pull_request_valid_removes_label(self, bot_env): + bot = IssueAssignmentBot() + bot.event_name = "pull_request_target" + bot.load_event_payload({ + "action": "edited", + "pull_request": { + "number": 100, + "user": {"login": "testuser"}, + "body": "Fixes #123", + } + }) + + mock_label = Mock() + mock_label.name = "invalid" + mock_pr_obj = Mock() + mock_pr_obj.labels = [mock_label] + bot_env["repo"].get_pull.return_value = mock_pr_obj + + with patch.object(bot, "validate_pr_issues", return_value=True): + assert bot.handle_pull_request() + mock_pr_obj.remove_from_labels.assert_called_once_with("invalid") + diff --git a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py index 20892a3a3..547286c5a 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -242,3 +242,86 @@ def test_no_github_client(self, bot_env): bot.github = None bot.repo = None assert not bot.run() + + +class TestStalePRBotInvalidCheck: + def test_invalid_pr_becomes_valid(self, bot_env): + bot = StalePRBot() + mock_label = Mock() + mock_label.name = "invalid" + + mock_pr = Mock() + mock_pr.labels = [mock_label] + mock_pr.number = 100 + bot_env["repo"].get_pulls.return_value = [mock_pr] + + with patch.object(bot, "validate_pr_issues", return_value=True): + assert bot.process_stale_prs() + mock_pr.remove_from_labels.assert_called_once_with("invalid") + mock_pr.edit.assert_not_called() + + def test_invalid_pr_still_invalid_under_24h(self, bot_env): + bot = StalePRBot() + mock_label = Mock() + mock_label.name = "invalid" + + mock_pr = Mock() + mock_pr.labels = [mock_label] + mock_pr.number = 100 + bot_env["repo"].get_pulls.return_value = [mock_pr] + + mock_comment = Mock() + mock_comment.user.type = "Bot" + mock_comment.body = " Warning comment" + mock_comment.created_at = datetime.now(timezone.utc) + mock_pr.get_issue_comments.return_value = [mock_comment] + + with patch.object(bot, "validate_pr_issues", return_value=False): + assert bot.process_stale_prs() + mock_pr.remove_from_labels.assert_not_called() + mock_pr.edit.assert_not_called() + + def test_invalid_pr_still_invalid_over_24h_closes(self, bot_env): + bot = StalePRBot() + mock_label = Mock() + mock_label.name = "invalid" + + mock_pr = Mock() + mock_pr.labels = [mock_label] + mock_pr.number = 100 + bot_env["repo"].get_pulls.return_value = [mock_pr] + + mock_comment = Mock() + mock_comment.user.type = "Bot" + mock_comment.body = " Warning comment" + from datetime import timedelta + mock_comment.created_at = datetime.now(timezone.utc) - timedelta(hours=25) + mock_pr.get_issue_comments.return_value = [mock_comment] + + with patch.object(bot, "validate_pr_issues", return_value=False): + assert bot.process_stale_prs() + mock_pr.remove_from_labels.assert_not_called() + mock_pr.create_issue_comment.assert_called_once() + assert "automatically closed" in mock_pr.create_issue_comment.call_args[0][0] + mock_pr.edit.assert_called_once_with(state="closed") + + def test_invalid_pr_missing_warning_comment_posts_warning(self, bot_env): + bot = StalePRBot() + mock_label = Mock() + mock_label.name = "invalid" + + mock_pr = Mock() + mock_pr.labels = [mock_label] + mock_pr.number = 100 + mock_pr.user.login = "contributor" + bot_env["repo"].get_pulls.return_value = [mock_pr] + + mock_pr.get_issue_comments.return_value = [] + + with patch.object(bot, "validate_pr_issues", return_value=False): + assert bot.process_stale_prs() + mock_pr.remove_from_labels.assert_not_called() + mock_pr.create_issue_comment.assert_called_once() + assert "" in mock_pr.create_issue_comment.call_args[0][0] + mock_pr.edit.assert_not_called() + diff --git a/.github/actions/bot-autoassign/utils.py b/.github/actions/bot-autoassign/utils.py index 813580977..99d1e8aa7 100644 --- a/.github/actions/bot-autoassign/utils.py +++ b/.github/actions/bot-autoassign/utils.py @@ -63,3 +63,41 @@ def unassign_linked_issues_helper(repo, repository_name, pr_body, pr_author): except Exception as e: print(f"Error unassigning issue #{issue_number}: {e}") return unassigned_issues + + +def extract_all_linked_issues(pr_body, default_repo_full_name): + """Extract all linked issues (including cross-repository and URLs). + + Returns a list of tuples: (owner, repo_name, issue_number) + """ + if not pr_body: + return [] + # Pattern to match: + # 1. URL: https://github.com/owner/repo/issues/num + # 2. owner/repo#num + # 3. #num + pattern = ( + r"\b(?:fix(?:e[sd])?|close[sd]?|resolve[sd]?|relat(?:e[sd]?|ed)\s+to)\s*:?\s*" + r"(?:" + r"https://github\.com/([\w-]+)/([\w.-]+)/issues/(\d+)" + r"|([\w-]+)/([\w.-]+)#(\d+)" + r"|#(\d+)" + r")" + ) + matches = re.findall(pattern, pr_body, re.IGNORECASE) + results = [] + default_owner, default_repo = default_repo_full_name.split("/") + for m in matches: + if m[0] and m[1] and m[2]: # URL + results.append((m[0], m[1], int(m[2]))) + elif m[3] and m[4] and m[5]: # owner/repo#num + results.append((m[3], m[4], int(m[5]))) + elif m[6]: # #num + results.append((default_owner, default_repo, int(m[6]))) + # Remove duplicates while preserving order + unique_results = [] + for r in results: + if r not in unique_results: + unique_results.append(r) + return unique_results + diff --git a/.github/workflows/bot-autoassign-pr-issue-link.yml b/.github/workflows/bot-autoassign-pr-issue-link.yml index d79c4218c..c50ae0d03 100644 --- a/.github/workflows/bot-autoassign-pr-issue-link.yml +++ b/.github/workflows/bot-autoassign-pr-issue-link.yml @@ -2,7 +2,7 @@ name: PR Issue Auto-Assignment on: pull_request_target: - types: [opened, reopened, closed] + types: [opened, reopened, closed, edited, ready_for_review] permissions: contents: read diff --git a/docs/developer/reusable-github-utils.rst b/docs/developer/reusable-github-utils.rst index 79fb22f2f..be270171e 100644 --- a/docs/developer/reusable-github-utils.rst +++ b/docs/developer/reusable-github-utils.rst @@ -72,6 +72,12 @@ OpenWISP repositories. The bot provides the following features: marks stale and unassigns after 14 days, and closes after 60 days. - **PR reopen reassignment**: When a stale PR is reopened, linked issues are reassigned back to the author. +- **PR validation**: Flags invalid pull requests from external contributors + that do not reference a validated issue belonging to the OpenWISP organization + (an open issue assigned to one of the required project boards with at least + one label other than ``invalid`` or ``wontfix``). Invalid pull requests are + labeled ``invalid``, receive a warning comment, and are automatically closed + after 24 hours if the issue is not resolved. **Secrets** @@ -132,7 +138,7 @@ Create the following workflow files in your repository. name: PR Issue Auto-Assignment on: pull_request_target: - types: [opened, reopened, closed] + types: [opened, reopened, closed, edited, ready_for_review] permissions: contents: read issues: write