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

Jump to bottom

warn if the PR Author does not have valid config setup #181

Merged
merged 3 commits into from
Feb 17, 2018
Merged

warn if the PR Author does not have valid config setup #181

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d1bedb
pr_summary: warn if the PR Author does have valid config setup
priyank-p Feb 16, 2018
7d3aa59
[squash] fix warning msg.
priyank-p Feb 16, 2018
4d5485c
[squash] rename incorrectConfigPR -> emptyProfilePR
priyank-p Feb 17, 2018
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
12 changes: 10 additions & 2 deletions lib/pr_summary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,16 @@ class PRSummary {
cli.table('Title', `${title} (#${prid})`);
const authorHint =
this.data.authorIsNew() ? ', first-time contributor' : '';
cli.table('Author',
`${author.name} <${author.email}> (@${author.login}${authorHint})`);

if (author.name && author.email) {
cli.table('Author',
`${author.name} <${author.email}> (@${author.login}${authorHint})`);
} else {
// user do not have correct `git config` setup
Copy link
Member

Choose a reason for hiding this comment

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

I think the information here comes from their github account settings? The git config setup are only visible in the commit information.

Take a look at the profile page https://github.com/antoine-amara and they don't have the name nor the email setup.

Copy link
Contributor Author

@priyank-p priyank-p Feb 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah actually not too sure about i guessed it since if you look at the issues the commit the user reference have correct config of commit.

Edit: issue link nodejs/node#18662

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look at the profile page https://github.com/antoine-amara and they don't have the name nor the email setup.

I don't think that matters see the issue link and the commits that are referenced they seem to perfect commit except the last one (which was the one in the PR).

Copy link
Member

@joyeecheung joyeecheung Feb 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it's possible to get an empty git config, since git should prompt for a username and a email when you commit for the first time. So the commits should always come with a name and a email (could be totally bogus, but still). It is possible to have empty github profile settings if you don't put those down during github account registration, or delete them afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See that something i am unsure about too. I have no idea how that occurred at the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like the user squashed a that commit so it might be that he changed his email causing the commit to empty details. Since it does have users profile picture.

Copy link
Member

Choose a reason for hiding this comment

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

@cPhost Hmm..maybe the user has set the email to private? I believe that would cause the email queried in the API to be null but Github still knows how to associate the commits with the account since they know about the private email address. Either way the warning should be something like "Could not retrieve the email from the PR author's GitHub profile"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, cool i can update the warning, also the name is also not present so i will add that in too.

cli.warn('The author of the PR does not have ' +
'correct email and name setup locally!');
}

cli.table('Branch', `${branch}`);
cli.table('Labels', `${labelStr}`);

Expand Down
2 changes: 2 additions & 0 deletions test/fixtures/data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ const firstTimerPrivatePR = readJSON('first_timer_pr_with_private_email.json');
const semverMajorPR = readJSON('semver_major_pr.json');
const fixAndRefPR = readJSON('pr_with_fixes_and_refs.json');
const conflictingPR = readJSON('conflicting_pr.json');
const incorrectConfigPR = readJSON('incorrect_config_pr.json');
Copy link
Member

Choose a reason for hiding this comment

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

Maybe renaming this to something like emptyProfilePR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah makes sense since this seems to be something other than incorrect config i was thinking.

const closedPR = readJSON('./closed_pr.json');
const mergedPR = readJSON('./merged_pr.json');
const readme = readFile('./README/README.md');
Expand Down Expand Up @@ -90,6 +91,7 @@ module.exports = {
semverMajorPR,
fixAndRefPR,
conflictingPR,
incorrectConfigPR,
readme,
readmeNoTsc,
readmeNoTscE,
Expand Down
22 changes: 22 additions & 0 deletions test/fixtures/incorrect_config_pr.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"createdAt": "2017-10-24T11:13:43Z",
"authorAssociation": "COLLABORATOR",
"author": {
"login": "pr_author",
"email": "",
"name": null
},
"url": "https://github.com/nodejs/node/pull/18721",
"bodyHTML": "<p>Fix mdn links</p>",
"bodyText": "Fix mdn links",
"labels": {
"nodes": [
{
"name": "doc"
}
]
},
"title": "doc: fix mdn links",
"baseRefName": "master",
"headRefName": "fix-links"
}
36 changes: 35 additions & 1 deletion test/unit/pr_summary.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ const {
oddCommits,
simpleCommits,
firstTimerPR,
semverMajorPR
semverMajorPR,
incorrectConfigPR
} = require('../fixtures/data');
const TestCLI = require('../fixtures/test_cli');
const PRSummary = require('../../lib/pr_summary');
Expand Down Expand Up @@ -86,4 +87,37 @@ describe('PRSummary', () => {
summary.display();
cli.assertCalledWith(expectedLogs);
});

it('displays warning if pr author/email is not present', () => {
const cli = new TestCLI();
const prData = {
pr: incorrectConfigPR,
commits: simpleCommits,
authorIsNew() {
return false;
}
};

const expectedLogs = {
log: [
[' - doc: some changes'],
[' - Their Github Account email <[email protected]>']
],
table: [
['Title', 'doc: fix mdn links (#16348)'],
['Branch', 'pr_author:fix-links -> nodejs:master'],
['Labels', 'doc'],
['Commits', '1'],
['Committers', '1']
],
warn: [
['The author of the PR does not have correct ' +
'email and name setup locally!']
]
};

const summary = new PRSummary(argv, cli, prData);
summary.display();
cli.assertCalledWith(expectedLogs);
});
});