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

Oppiabot is saying that contributors can't merge PRs, when they can. #261

Open
seanlip opened this issue May 8, 2021 · 1 comment
Open
Labels
bug Something isn't working Impact: High Work: Low

Comments

@seanlip
Copy link
Member

seanlip commented May 8, 2021

Describe the bug

On oppia/oppia#12620 (comment), Oppiabot is saying:

"Hi @aks681, this PR is ready to be merged. Author of this PR does not have permissions to merge this PR. Before you merge it, please make sure that there are no pending comments that require action from the author's end. Thanks!"

However, this PR is authored by Rijuta, who is a member of the Oppia organization and does indeed have permissions to merge PRs. So, this seems like a bug with Oppiabot.

Observed behavior
Oppiabot posts the following message erroneously: "Hi @aks681, this PR is ready to be merged. Author of this PR does not have permissions to merge this PR. Before you merge it, please make sure that there are no pending comments that require action from the author's end. Thanks!"

Expected behavior
Oppiabot should not post this message because the PR author has permissions to merge the PR.

@Mayank77maruti
Copy link

I have identified that the referenced message no longer exists in the Oppiabot codebase. This removal occurred as part of the following commit:

80eb0f2#diff-5c3e6c40b35fd444459b7bb6c629ddd1ac358ec4eb79f9f341c7a60899d63e66

Previously, this message was located in the spec/checkPullRequestReviewSpec.js file:
image

Current Code

Now, the relevant section of the code looks as follows:

// Check if author can merge PR. All members can merge the pull request.
const authorCanMerge = await utilityModule.hasMergePrivilege(
  context, pullRequest.user.login
);

let commentBody;
if (authorCanMerge) {
  // Ping and assign author.
  commentBody =
    'Hi @' +
    pullRequest.user.login +
    ', this PR is ready to be merged. Please address any remaining ' +
    'comments prior to merging, and feel free to merge this PR ' +
    "once the CI checks pass and you're happy with it. Thanks!";
} else {
  // Ping and assign author.
  commentBody =
    'Hi @' + pullRequest.user.login + ', this PR is ready to be merged. ' +
    'Please address any remaining comments prior to merging, and feel free ' +
    'to ask someone to merge your PR once the CI checks pass and ' +
    'you\'re happy with it. Thanks!';
}

Observations

The if and else blocks appear almost identical. Ideally, there should be a difference to avoid redundancy.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Impact: High Work: Low
Projects
Status: Todo
Development

No branches or pull requests

2 participants