diff --git a/Bugzilla/API/V1/Github.pm b/Bugzilla/API/V1/Github.pm index 0dd7d0aa80..c620c5bf5c 100644 --- a/Bugzilla/API/V1/Github.pm +++ b/Bugzilla/API/V1/Github.pm @@ -28,6 +28,11 @@ use Mojo::Util qw(secure_compare); use constant BUG_RE => qr/\b[Bb]ug(?:[ -]|: )(\d+)\b/; +# Commit titles that undo earlier work. A push with any of these for a bug +# must not close it. Anchored to the start of the title so a normal +# commit that merely mentions the word "revert" still closes the bug. +use constant REVERT_RE => qr/^(?:Revert\b|Backed out (?:\d+ )?changesets?\b)/i; + sub setup_routes { my ($class, $r) = @_; $r->post('/github/pull_request')->to('V1::Github#pull_request'); @@ -309,7 +314,8 @@ sub push_comment { my $comment_text = "Authored by $author\n$url\n[$branch] $message"; $update_bugs{$bug_id} ||= []; - push @{$update_bugs{$bug_id}}, {text => $comment_text}; + push @{$update_bugs{$bug_id}}, + {text => $comment_text, is_revert => $message =~ REVERT_RE ? 1 : 0}; } # Restrict this request to the bugs the authenticated webhook credential can @@ -376,6 +382,21 @@ sub push_comment { $comment_text .= $comment->{text} . "\n\n"; } + # Close the bug as RESOLVED/FIXED unless it has the 'leave-open' keyword + # or is already closed. If any commit for this bug is a revert/backout, + # leave it open and say why, since it would otherwise have been closed. + # A push can carry a fix and a revert of that fix together, so one revert + # is enough to make "fixed" unsafe to assume. + my $close_bug + = !$bug->has_keyword('leave-open') + && $bug->status ne 'RESOLVED' + && $bug->status ne 'VERIFIED'; + if ($close_bug && grep { $_->{is_revert} } @{$update_bugs{$bug_id}}) { + $close_bug = 0; + $comment_text .= "Bug left open because one or more of the pushed commits " + . "is a revert or backout.\n\n"; + } + # Attribute the automated comment to the webhook credential that triggered # it, so the bot behind a github-automation action is visible in bug history # (not just server logs) and any leaked-key abuse is traceable on the bug. @@ -388,11 +409,7 @@ sub push_comment { } }; - # If the bug does not have the keyword 'leave-open', we close the bug as RESOLVED/FIXED. - if (!$bug->has_keyword('leave-open') - && $bug->status ne 'RESOLVED' - && $bug->status ne 'VERIFIED') - { + if ($close_bug) { # Set the bugs status to RESOLVED/FIXED $set_all->{bug_status} = 'RESOLVED'; $set_all->{resolution} = 'FIXED'; diff --git a/qa/t/rest_github_push_comment.t b/qa/t/rest_github_push_comment.t index af464f2f5d..9679c22b02 100644 --- a/qa/t/rest_github_push_comment.t +++ b/qa/t/rest_github_push_comment.t @@ -463,4 +463,99 @@ $t->put_ok($url ->json_is('/message' => 'You are not allowed to react to comments on this bug.'); +# Revert and backout commits must comment on the bug but not close it, and the +# comment must say why the bug was left open. +my $revert_note + = "\n\nBug left open because one or more of the pushed commits " + . "is a revert or backout."; + +sub create_test_bug { + my ($summary) = @_; + $t->post_ok($url + . 'rest/bug' => {'X-Bugzilla-API-Key' => $api_key} => json => { + product => 'Firefox', + component => 'General', + summary => $summary, + type => 'defect', + version => 'unspecified', + severity => 'blocker', + description => 'This is a new test bug', + })->status_is(200)->json_has('/id'); + return $t->tx->res->json->{id}; +} + +sub push_titles { + my ($bug_id, @titles) = @_; + my $payload = { + ref => 'refs/heads/master', + repository => { + full_name => 'mozilla-bteam/bmo', + default_branch => 'master', + }, + commits => [ + map { { + author => {username => 'foobar', name => 'Foo Bar'}, + url => "https://github.com/mozilla-bteam/bmo/commit/$_", + message => $titles[$_], + } } 0 .. $#titles + ], + }; + $t->post_ok( + $url + . 'rest/github/push_comment' => { + 'X-Hub-Signature-256' => generate_payload_signature($secret, $payload), + 'X-GitHub-Event' => 'push' + } => json => $payload + )->status_is(200)->json_has("/bugs/$bug_id/id"); + + my $comment_id = $t->tx->res->json->{bugs}->{$bug_id}->{id}; + my $expected = join "\n\n", + map {"Authored by https://github.com/foobar\n" + . "https://github.com/mozilla-bteam/bmo/commit/$_\n[master] $titles[$_]"} + 0 .. $#titles; + return ($comment_id, $expected); +} + +sub check_bug { + my ($bug_id, $comment_id, $expected_text, $status) = @_; + $t->get_ok( + $url . "rest/bug/comment/$comment_id" => {'X-Bugzilla-API-Key' => $api_key}) + ->status_is(200)->json_is("/comments/$comment_id/text", $expected_text); + $t->get_ok($url + . "rest/bug/$bug_id?include_fields=id,status" => + {'X-Bugzilla-API-Key' => $api_key})->status_is(200) + ->json_is('/bugs/0/status', $status); +} + +foreach my $title ( + 'Revert "Bug %d - Some change"', + 'Backed out changeset abcdef123456 (Bug %d) for test failures', + 'Backed out 2 changesets (Bug %d) for test failures', + ) +{ + my $revert_bug_id = create_test_bug('Test GitHub Push Commenting (revert)'); + my ($comment_id, $expected) + = push_titles($revert_bug_id, sprintf($title, $revert_bug_id)); + check_bug($revert_bug_id, $comment_id, + $expected . $revert_note . $comment_trailer, 'CONFIRMED'); +} + +# A push mixing a fix and a revert for the same bug leaves it open +# (bug 2084270 comment 9 was closed by exactly this kind of push). +my $mixed_bug_id = create_test_bug('Test GitHub Push Commenting (mixed)'); +my ($mixed_comment_id, $mixed_expected) = push_titles( + $mixed_bug_id, + "Bug $mixed_bug_id - First attempt", + "Revert \"Bug $mixed_bug_id - First attempt\"" +); +check_bug($mixed_bug_id, $mixed_comment_id, + $mixed_expected . $revert_note . $comment_trailer, 'CONFIRMED'); + +# "revert" appearing later in the title is a normal commit and closes the bug. +my $word_bug_id = create_test_bug('Test GitHub Push Commenting (revert word)'); +my ($word_comment_id, $word_expected) + = push_titles($word_bug_id, "Bug $word_bug_id - Revert button styling fix"); +check_bug($word_bug_id, $word_comment_id, $word_expected . $comment_trailer, + 'RESOLVED'); + done_testing();