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.

Sign up for GitHub

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

[NO QA]Fix two bugs found with checklist tests #10834

Merged
merged 2 commits into from
Sep 6, 2022
Merged

[NO QA]Fix two bugs found with checklist tests #10834

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
281b7bb
Fix two bugs found with checklists
AndrewGable Sep 6, 2022
7e34c84
Be explicit in which types to run on
AndrewGable Sep 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 29 additions & 31 deletions .github/actions/javascript/contributorChecklist/contributorChecklist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const core = require('@actions/core');
const github = require('@actions/github');
const _ = require('underscore');
const GitHubUtils = require('../../../libs/GithubUtils');

/* eslint-disable max-len */
Expand Down Expand Up @@ -99,40 +100,39 @@ const completedContributorPlusChecklist = `- [x] I have verified the author chec
const issue = github.context.payload.issue ? github.context.payload.issue.number : github.context.payload.pull_request.number;
const combinedData = [];

function printUncheckedItems(result) {
const checklist = result.split('\n');
function getPullRequestBody() {
return GitHubUtils.octokit.pulls.get({
owner: GitHubUtils.GITHUB_OWNER,
repo: GitHubUtils.APP_REPO,
pull_number: issue,
}).then(({data: pullRequestComment}) => pullRequestComment.body);
}

checklist.forEach((line) => {
// Provide a search string with the first 30 characters to figure out if the checkbox item is in the checklist
const lineSearchString = line.replace('- [ ] ', '').slice(0, 30);
if (line.includes('- [ ]') && (completedContributorChecklist.includes(lineSearchString) || completedContributorPlusChecklist.includes(lineSearchString))) {
console.log(`Unchecked checklist item: ${line}`);
}
});
function getAllReviewComments() {
return GitHubUtils.paginate(GitHubUtils.octokit.pulls.listReviews, {
owner: GitHubUtils.GITHUB_OWNER,
repo: GitHubUtils.APP_REPO,
pull_number: issue,
per_page: 100,
}, response => _.map(response.data, review => review.body));
}

// Get all user text from the pull request, review comments, and pull request comments
GitHubUtils.octokit.pulls.get({
owner: GitHubUtils.GITHUB_OWNER,
repo: GitHubUtils.APP_REPO,
pull_number: issue,
}).then(({data: pullRequestComment}) => {
combinedData.push(pullRequestComment.body);
}).then(() => GitHubUtils.octokit.pulls.listReviews({
owner: GitHubUtils.GITHUB_OWNER,
repo: GitHubUtils.APP_REPO,
pull_number: issue,
})).then(({data: pullRequestReviewComments}) => {
pullRequestReviewComments.forEach(pullRequestReviewComment => combinedData.push(pullRequestReviewComment.body));
})
.then(() => GitHubUtils.octokit.issues.listComments({
function getAllComments() {
return GitHubUtils.paginate(GitHubUtils.octokit.issues.listComments, {
owner: GitHubUtils.GITHUB_OWNER,
repo: GitHubUtils.APP_REPO,
issue_number: issue,
per_page: 100,
}))
.then(({data: pullRequestComments}) => {
pullRequestComments.forEach(pullRequestComment => combinedData.push(pullRequestComment.body));
}, response => _.map(response.data, comment => comment.body));
}

getPullRequestBody()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use Promise.all to fire off getPullRequestBody, getAllReviewComments, and getAllComments at the same time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but I don't see a huge benefit

.then(pullRequestBody => combinedData.push(pullRequestBody))
.then(() => getAllReviewComments())
.then(reviewComments => combinedData.push(...reviewComments))
.then(() => getAllComments())
.then(comments => combinedData.push(...comments))
.then(() => {
let contributorChecklistComplete = false;
let contributorPlusChecklistComplete = false;

Expand All @@ -143,23 +143,21 @@ GitHubUtils.octokit.pulls.get({

if (comment.includes(completedContributorChecklist.replace(whitespace, ''))) {
contributorChecklistComplete = true;
} else if (comment.includes('- [')) {
printUncheckedItems(combinedData[i]);
}

if (comment.includes(completedContributorPlusChecklist.replace(whitespace, ''))) {
contributorPlusChecklistComplete = true;
} else if (comment.includes('- [')) {
printUncheckedItems(combinedData[i]);
}
}

if (!contributorChecklistComplete) {
console.log('Make sure you are using the most up to date checklist found here: https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md');
core.setFailed('Contributor checklist is not completely filled out. Please check every box to verify you\'ve thought about the item.');
return;
}

if (!contributorPlusChecklistComplete) {
console.log('Make sure you are using the most up to date checklist found here: https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md');
core.setFailed('Contributor plus checklist is not completely filled out. Please check every box to verify you\'ve thought about the item.');
return;
}
Expand Down
60 changes: 29 additions & 31 deletions .github/actions/javascript/contributorChecklist/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ module.exports =

const core = __nccwpck_require__(2186);
const github = __nccwpck_require__(5438);
const _ = __nccwpck_require__(3571);
const GitHubUtils = __nccwpck_require__(7999);

/* eslint-disable max-len */
Expand Down Expand Up @@ -109,40 +110,39 @@ const completedContributorPlusChecklist = `- [x] I have verified the author chec
const issue = github.context.payload.issue ? github.context.payload.issue.number : github.context.payload.pull_request.number;
const combinedData = [];

function printUncheckedItems(result) {
const checklist = result.split('\n');
function getPullRequestBody() {
return GitHubUtils.octokit.pulls.get({
owner: GitHubUtils.GITHUB_OWNER,
repo: GitHubUtils.APP_REPO,
pull_number: issue,
}).then(({data: pullRequestComment}) => pullRequestComment.body);
}

checklist.forEach((line) => {
// Provide a search string with the first 30 characters to figure out if the checkbox item is in the checklist
const lineSearchString = line.replace('- [ ] ', '').slice(0, 30);
if (line.includes('- [ ]') && (completedContributorChecklist.includes(lineSearchString) || completedContributorPlusChecklist.includes(lineSearchString))) {
console.log(`Unchecked checklist item: ${line}`);
}
});
function getAllReviewComments() {
return GitHubUtils.paginate(GitHubUtils.octokit.pulls.listReviews, {
owner: GitHubUtils.GITHUB_OWNER,
repo: GitHubUtils.APP_REPO,
pull_number: issue,
per_page: 100,
}, response => _.map(response.data, review => review.body));
}

// Get all user text from the pull request, review comments, and pull request comments
GitHubUtils.octokit.pulls.get({
owner: GitHubUtils.GITHUB_OWNER,
repo: GitHubUtils.APP_REPO,
pull_number: issue,
}).then(({data: pullRequestComment}) => {
combinedData.push(pullRequestComment.body);
}).then(() => GitHubUtils.octokit.pulls.listReviews({
owner: GitHubUtils.GITHUB_OWNER,
repo: GitHubUtils.APP_REPO,
pull_number: issue,
})).then(({data: pullRequestReviewComments}) => {
pullRequestReviewComments.forEach(pullRequestReviewComment => combinedData.push(pullRequestReviewComment.body));
})
.then(() => GitHubUtils.octokit.issues.listComments({
function getAllComments() {
return GitHubUtils.paginate(GitHubUtils.octokit.issues.listComments, {
owner: GitHubUtils.GITHUB_OWNER,
repo: GitHubUtils.APP_REPO,
issue_number: issue,
per_page: 100,
}))
.then(({data: pullRequestComments}) => {
pullRequestComments.forEach(pullRequestComment => combinedData.push(pullRequestComment.body));
}, response => _.map(response.data, comment => comment.body));
}

getPullRequestBody()
.then(pullRequestBody => combinedData.push(pullRequestBody))
.then(() => getAllReviewComments())
.then(reviewComments => combinedData.push(...reviewComments))
.then(() => getAllComments())
.then(comments => combinedData.push(...comments))
.then(() => {
let contributorChecklistComplete = false;
let contributorPlusChecklistComplete = false;

Expand All @@ -153,23 +153,21 @@ GitHubUtils.octokit.pulls.get({

if (comment.includes(completedContributorChecklist.replace(whitespace, ''))) {
contributorChecklistComplete = true;
} else if (comment.includes('- [')) {
printUncheckedItems(combinedData[i]);
}

if (comment.includes(completedContributorPlusChecklist.replace(whitespace, ''))) {
contributorPlusChecklistComplete = true;
} else if (comment.includes('- [')) {
printUncheckedItems(combinedData[i]);
}
}

if (!contributorChecklistComplete) {
console.log('Make sure you are using the most up to date checklist found here: https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md');
core.setFailed('Contributor checklist is not completely filled out. Please check every box to verify you\'ve thought about the item.');
return;
}

if (!contributorPlusChecklistComplete) {
console.log('Make sure you are using the most up to date checklist found here: https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md');
core.setFailed('Contributor plus checklist is not completely filled out. Please check every box to verify you\'ve thought about the item.');
return;
}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/testChecklists.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ jobs:
checklist:
runs-on: ubuntu-latest
# Only run when a comment, PR, or PR review comment contains a checklist item
if: ${{ github.actor != 'OSBotify' || (github.event.issue.pull_request && github.event_name == 'issue_comment' && contains(github.event.comment.body, '- [') || github.event_name == 'pull_request_review' && contains(github.event.review.body, '- [') || github.event_name == 'pull_request' && contains(github.event.pull_request.body, '- [')) }}
if: github.actor != 'OSBotify' && (contains(github.event.issue.pull_request.url, 'http') && github.event_name == 'issue_comment' && contains(github.event.comment.body, '- [') || github.event_name == 'pull_request_review' && contains(github.event.review.body, '- [') || github.event_name == 'pull_request' && contains(github.event.pull_request.body, '- ['))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the deal here? When does pull_request event not have an http url?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So GitHub suggests to check ${{ github.event.issue.pull_request }} in their docs. pull_request is an optional object that appears on PRs and not on issues. Checking for it in this way with additional logic doesn't seem to work as expected because it returns undefined instead of a boolean. I changed to check for pull_request.url containing http as that returns a boolean always.

steps:
- name: contributorChecklist.js
uses: Expensify/App/.github/actions/javascript/contributorChecklist@main
Expand Down