Skip to content
This repository has been archived by the owner on Aug 27, 2024. It is now read-only.

ISSUE-544: Add support for native approvals, fix validation bug, fix typos #586

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

ISSUE-544: Add support for native approvals, fix validation bug, fix typos #586

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
dc45e0b
ISSUE-544: Add support for native approvals, fix validation bug, fix …
GeorgySerga Oct 25, 2019
2c867f6
ISSUE-544: Add pull_request fallback and getReviews mock for tests
GeorgySerga Nov 1, 2019
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
4 changes: 2 additions & 2 deletions common/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,9 @@ export function symbolToString(sym) {
}

export function toGenericComment(githubComment) {
const {user, ...rest} = githubComment
const {user: { login: user }, ...rest} = githubComment
return {
...rest,
user: user.login
user,
}
}
181 changes: 112 additions & 69 deletions server/checks/Approval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export default class Approval extends Check {
static TYPE = 'approval'
static CONTEXT = context
static NAME = 'Approval check'
static HOOK_EVENTS = [EVENTS.PULL_REQUEST, EVENTS.ISSUE_COMMENT]
static HOOK_EVENTS = [EVENTS.PULL_REQUEST, EVENTS.ISSUE_COMMENT, EVENTS.PULL_REQUEST_REVIEW]
Copy link
Author

Choose a reason for hiding this comment

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

A new event is added to ask GitHub to notify on PULL_REQUEST_REVIEW events.


/**
* @param {GithubService} github
Expand All @@ -48,7 +48,7 @@ export default class Approval extends Check {
* and returns it if it exists. Otherwise creates and returns a new one.
*
* @param dbRepoId The db id for the repository
* @param {RepositoryHandler} Numbe/github identifier of the pull request
* @param number github identifier of the pull request
* @returns {Object} The pull request information stored/created in the database.
*/
async getOrCreateDbPullRequest(dbRepoId, number) {
Expand Down Expand Up @@ -118,39 +118,38 @@ export default class Approval extends Check {
}

/**
* Returns the comment if it matches the config, null otherwise.
* Returns `true` if username is in the config, false `otherwise`.
Copy link
Author

Choose a reason for hiding this comment

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

Just for consistency, comment wasn't used, but return value was checked inside conditions. Seems like booleans are more suitable for that.

*/
async doesCommentMatchConfig(repository, comment, fromConfig, token) {
async doesUsernameMatchConfig(repository, username, fromConfig, token) {
// persons must either be listed explicitly in users OR
// be a collaborator OR
// member of at least one of listed orgs
const {orgs, collaborators, users} = fromConfig
const username = comment.user
const {full_name} = repository
// first do the quick username check
if (users && users.indexOf(username) >= 0) {
debug(`${full_name}: ${username} is listed explicitly`)
return comment
return true
}
// now collaborators
if (collaborators) {
const isCollaborator = await this.github.isCollaborator(repository.owner.login, repository.name, username, token)
if (isCollaborator) {
debug(`${full_name}: ${username} is collaborator`)
return comment
return true
}
}
// and orgs
if (orgs) {
const orgMember = await Promise.all(orgs.map(o => this.github.isMemberOfOrg(o, username, token)))
if (orgMember.indexOf(true) >= 0) {
debug(`${full_name}: ${username} is org member`)
return comment
return true
}
}
debug(`${full_name}: ${username}'s approval does not count`)
// okay, no member of anything
return null
return false
}

/**
Expand All @@ -168,7 +167,7 @@ export default class Approval extends Check {
async function checkComment(stats, comment) {
let matchesTotal = false
if (config.from) {
matchesTotal = await that.doesCommentMatchConfig(repository, comment, config.from, token)
matchesTotal = await that.doesUsernameMatchConfig(repository, comment.user, config.from, token)
if (matchesTotal) {
info(`${repository.full_name}: Counting ${comment.user}'s comment`)
stats.total.push(comment.user)
Expand All @@ -184,7 +183,7 @@ export default class Approval extends Check {
if (!stats.groups[group]) {
stats.groups[group] = []
}
const matchesGroup = await that.doesCommentMatchConfig(repository, comment, config.groups[group].from, token)
const matchesGroup = await that.doesUsernameMatchConfig(repository, comment.user, config.groups[group].from, token)
if (matchesGroup) {
// counting this as total as well if it didn't before
if (!matchesTotal) {
Expand All @@ -209,63 +208,93 @@ export default class Approval extends Check {
*
* @param repository The repository
* @param pull_request The pull request
* @param comments The comments to process
* @param entries The comments or reviews to process
* @param config The approval configuration
* @param token The access token to use
* @returns {Object} Object of the shape {approvals: {total: int, groups: { groupName: int }}, vetos: int }
*/
async countApprovalsAndVetos(repository, pull_request, comments, config, token) {
async countApprovalsAndVetos(repository, pull_request, entries, config, token) {
const ignore = await this.fetchIgnoredUsers(repository, pull_request, config, token)
const approvalPattern = config.pattern
const vetoPattern = _.get(config, 'veto.pattern')

const fullName = `${repository.full_name}`
// slightly unperformant filtering here:
const containsAlreadyCommentByUser = (c1, i, cmts) => i === cmts.findIndex(c2 => c1.user === c2.user)
// filter ignored users
const potentialApprovalComments = comments.filter(comment => {
const login = comment.user
// comment is ignored if login is in ignored list or matches a global ignored regex array
const include = (ignore.indexOf(login) === -1 && commenterIsNotIgnored(login));
if (!include) {
info('%s: Ignoring user: %s.', fullName, login)
}
return include
})
// get comments that match specified approval pattern
// TODO add unicode flag once available in node
.filter(comment => {
const text = comment.body.trim()
const include = (new RegExp(approvalPattern)).test(text)
if (!include) {
info('%s: %s\'s comment "%s" does not match pattern "%s".', fullName, comment.user, text.substring(0, 16), approvalPattern)
}
return include
})
// kicking out multiple approvals from same person
.filter(containsAlreadyCommentByUser)

const containsAlreadyEntryByUser = (c1, i, cmts) => i === cmts.findIndex(c2 => c1.user === c2.user)
const potentialApprovals = entries.filter(entry => {
const login = entry.user
// entry is ignored if login is in ignored list or matches a global ignored regex array
const include = (ignore.indexOf(login) === -1 && commenterIsNotIgnored(login));
if (!include) {
info('%s: Ignoring user: %s.', fullName, login)
}
return include
})
// get comments that match specified approval pattern
// TODO add unicode flag once available in node
.filter(entry => {
const text = entry.body.trim()
if (entry.state) {
// `state` is available only for 'reviews' and is not for 'comments'
return entry.state === 'APPROVED'
} else {
const include = (new RegExp(approvalPattern)).test(text)
if (!include) {
info('%s: %s\'s comment "%s" does not match pattern "%s".', fullName, entry.user, text.substring(0, 16), approvalPattern)
}
return include
}
})
// reverse the order to preserve newest entries
.reverse()
// kicking out multiple approvals from same person
.filter(containsAlreadyEntryByUser)

const potentialVetos = vetoPattern ?
entries.filter(entry => {
if (entry.state) {
// `state` is available only for 'reviews' and is not for 'comments'
return entry.state === 'CHANGES_REQUESTED'
} else {
const text = entry.body.trim()
const include = (new RegExp(vetoPattern)).test(text)
return include
}
})
// reverse the order to preserve newest entries
.reverse()
// kicking out multiple vetos from same person
.filter(containsAlreadyEntryByUser) : []

/**
* At this point we want to filter `potentialVetos` and `potentialApprovals`
* once more, based on selection in them. Approval can filter +1 out, as well as code change
* requirement should drop approvals
*/

const approvalsToUse = potentialApprovals.filter(a => {
const approvalTime = a.submitted_at || a.created_at
return potentialVetos.every(v => {
const vetoTime = v.submitted_at || v.created_at
return new Date(vetoTime) < new Date(approvalTime) || a.user !== v.user
})
})

const vetosToUse = potentialVetos.filter(v => {
const vetoTime = v.submitted_at || v.created_at
return potentialApprovals.every(a => {
const approvalTime = a.submitted_at || a.created_at
return new Date(vetoTime) > new Date(approvalTime) || a.user !== v.user
})
})
Copy link
Author

Choose a reason for hiding this comment

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

This is probably not the most efficient way to filter but IMO rather explicit. As the number of comments wouldn't be any large, these operations would be quick. This can be extracted to a separate function on the one hand, on the other it would be slightly weird to pass comparison operation inside, while these wouldn't be reused anywhere else. Might be a good idea to extract though, to to unit test later on.

This part also fixes (what I think is) a bug, when a user leaves-1 and then adds +1 build would be blocked unless new commit is pushed.


const approvals = (config.from || config.groups) ?
await this.getCommentStatsForConfig(repository, potentialApprovalComments, config, token) :
{total: potentialApprovalComments.map(c => c.user)}


let vetos = []
if (vetoPattern) {
const potentialVetoComments = comments.filter(comment => {
const text = comment.body.trim()
const include = (new RegExp(vetoPattern)).test(text)
return include
})
// kicking out multiple vetos from same person
.filter(containsAlreadyCommentByUser)
await this.getCommentStatsForConfig(repository, approvalsToUse, config, token) :
{total: approvalsToUse.map(c => c.user)}

vetos = (config.from || config.groups) ?
(await this.getCommentStatsForConfig(repository, potentialVetoComments, config, token)).total :
potentialVetoComments.map(c => c.user)

}
const vetos = vetoPattern ? (config.from || config.groups) ?
(await this.getCommentStatsForConfig(repository, vetosToUse, config, token)).total :
vetosToUse.map(c => c.user) : []

return {
approvals,
Expand Down Expand Up @@ -323,7 +352,19 @@ export default class Approval extends Check {
const user = repository.owner.login
const upstreamComments = await this.github.getComments(user, repository.name, pull_request.number, formatDate(last_push), token)
const comments = [...frozenComments, ...upstreamComments].filter((c, idx, array) => idx === array.findIndex(c2 => c.id === c2.id))
return await this.countApprovalsAndVetos(repository, pull_request, comments, config.approvals, token)

const upstreamReviews = await this.github.getReviews(user, repository.name, pull_request.number, formatDate(last_push), token)
Copy link
Author

Choose a reason for hiding this comment

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

As many other changes part getReviews is written to be consistent with getComments.


const entries = [...comments, ...upstreamReviews].sort((a, b) => {
Copy link
Author

Choose a reason for hiding this comment

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

Comments and reviews are being combined and sorted to check if any of the -1 are overwritten by the later native Approval.

// created_at for comments and submitted_at for reviews
const aTime = a.created_at || a.submitted_at
const bTime = b.created_at || b.submitted_at
return new Date(aTime) - new Date(bTime)
})

const approvalsAndVetos = await this.countApprovalsAndVetos(repository, pull_request, entries, config.approvals, token)

return approvalsAndVetos
}

/**
Expand Down Expand Up @@ -384,10 +425,10 @@ export default class Approval extends Check {
* @param dbRepoId The database ID of the affected repository
*/
async execute(config, event, hookPayload, token, dbRepoId) {
const {action, repository, pull_request, number, issue} = hookPayload
const {action, repository, pull_request, number = pull_request.number} = hookPayload
const repoName = repository.name
const user = repository.owner.login
const {minimum} = config.approvals
const { minimum } = config.approvals
let sha = ''
const pendingPayload = {
state: 'pending',
Expand Down Expand Up @@ -430,7 +471,7 @@ export default class Approval extends Check {
const approvals = {total: []}
const vetos = []
const status = Approval.generateStatus({approvals, vetos}, config.approvals)
await this.github.setCommitStatus(user, repoName, sha, status, token)
await this.github.setCommitStatus(user, repoName, sha, status, token) // TODO: seems unnecessary
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks redundant to me too

await this.audit.log(new AuditEvent(AUDIT_EVENTS.COMMIT_STATUS_UPDATE).fromGithubEvent(hookPayload)
.withResult({
approvals,
Expand Down Expand Up @@ -474,33 +515,34 @@ export default class Approval extends Check {
info(`${repository.full_name}#${number}: PR was synced, set state to pending`)
}
// on an issue comment
} else if (event === EVENTS.ISSUE_COMMENT) {
} else if (event === EVENTS.ISSUE_COMMENT || event === EVENTS.PULL_REQUEST_REVIEW) {
// check it belongs to an open pr
const pr = await this.github.getPullRequest(user, repoName, issue.number, token)
const pr = await this.github.getPullRequest(user, repoName, number, token)
if (!pr || pr.state !== 'open') {
debug(`${repository.full_name}#${issue.number}: Ignoring comment, not a PR`)
debug(`${repository.full_name}#${number}: Ignoring comment, not a PR`)
return
}
const author = hookPayload.comment.user.login

const author = hookPayload.comment ? hookPayload.comment.user.login : hookPayload.review.user.login
if (!commenterIsNotIgnored(author)) {
debug(`${repository.full_name}#${issue.number}: Ignoring comment, it was created by a robot user.`)
debug(`${repository.full_name}#${number}: Ignoring comment, it was created by a robot user.`)
return
}
sha = pr.head.sha
// set status to pending first
await this.github.setCommitStatus(user, repoName, sha, pendingPayload, token)
// read last push date from db
const dbPR = await this.getOrCreateDbPullRequest(dbRepoId, issue.number)
const dbPR = await this.getOrCreateDbPullRequest(dbRepoId, number)
// read frozen comments and update if appropriate
const frozenComments = await this.pullRequestHandler.onGetFrozenComments(dbPR.id, dbPR.last_push)
const commentId = hookPayload.comment.id
const commentId = hookPayload.comment ? hookPayload.comment.id : hookPayload.review.id
const commentAlreadyFrozen = frozenComments.map(comment => comment.id).indexOf(commentId) !== -1
if (['edited', 'deleted'].indexOf(action) !== -1 && !commentAlreadyFrozen) {
// check if it was edited by someone else than the original author
const editor = hookPayload.sender.login
if (editor !== author) {
// OMFG
const comment = toGenericComment(hookPayload.comment)
const comment = toGenericComment(hookPayload.comment || hookPayload.review)
const frozenComment = {
id: commentId,
body: action === 'edited' ? hookPayload.changes.body.from : comment.body,
Expand All @@ -511,15 +553,16 @@ export default class Approval extends Check {
if (new Date(comment.created_at) > dbPR.last_push) {
frozenComments.push(frozenComment)
}
info(`${repository.full_name}#${issue.number}: ${editor} ${action} ${author}'s comment ${commentId}, it's now frozen.`)
info(`${repository.full_name}#${number}: ${editor} ${action} ${author}'s comment ${commentId}, it's now frozen.`)
}
}
debug(`${repository.full_name}#${issue.number}: Comment added`)
debug(`${repository.full_name}#${number}: Comment added`)
await this.fetchApprovalsAndSetStatus(repository, pr, dbPR.last_push, config, token, frozenComments)
}
}
catch (e) {
error(e)
console.error({'`execute` error': e})
await this.github.setCommitStatus(user, repoName, sha, {
state: 'error',
context,
Expand Down
2 changes: 1 addition & 1 deletion server/handler/CheckHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export class CheckHandler {
*/
async onEnableCheck(user, repository, type) {
const repo = repository.get('json')
const types = [type, ...repository.checks.map(c => c.type)]
const types = Array.from(new Set([type, ...repository.checks.map(c => c.type)]))
Copy link
Author

Choose a reason for hiding this comment

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

I had an error from GitHub at some point, that multiple types where being sent. This just removes possible duplicates.

const events = findHookEventsFor(types)
// TODO: could use a database constraint instead?
let existingCheck
Expand Down
8 changes: 7 additions & 1 deletion server/model/GithubEvents.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export const PING = 'ping'
*/
export const COMMIT_COMMENT = 'commit_comment'
/**
* Any time a Commit is commented on.
* Any time a Commit is commented on. TODO: Mistake, copied from above?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, looks like a c&p error.

*/
export const CREATE = 'create'
/**
Expand Down Expand Up @@ -55,6 +55,12 @@ export const PAGE_BUILD = 'page_build'
* Any time a Repository changes from private to public.
*/
export const PUBLIC = 'public'
/**
* Triggered when a pull request review is submitted into a non-pending state, the body is edited,
* or the review is dismissed.
* https://developer.github.com/v3/activity/events/types/#pullrequestreviewevent
*/
export const PULL_REQUEST_REVIEW = 'pull_request_review'
/**
* Any time a comment is created on a portion of the unified diff of a pull request (the Files Changed tab).
*/
Expand Down
Loading