ISSUE-544: Add support for native approvals, fix validation bug, fix typos #586
Conversation
| @@ -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] | |||
There was a problem hiding this comment.
A new event is added to ask GitHub to notify on PULL_REQUEST_REVIEW events.
| @@ -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`. | |||
There was a problem hiding this comment.
Just for consistency, comment wasn't used, but return value was checked inside conditions. Seems like booleans are more suitable for that.
| const approvalTime = a.submitted_at || a.created_at | ||
| return new Date(vetoTime) > new Date(approvalTime) || a.user !== v.user | ||
| }) | ||
| }) |
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
As many other changes part getReviews is written to be consistent with getComments.
|
|
||
| const upstreamReviews = await this.github.getReviews(user, repository.name, pull_request.number, formatDate(last_push), token) | ||
|
|
||
| const entries = [...comments, ...upstreamReviews].sort((a, b) => { |
There was a problem hiding this comment.
Comments and reviews are being combined and sorted to check if any of the -1 are overwritten by the later native Approval.
| @@ -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)])) | |||
There was a problem hiding this comment.
I had an error from GitHub at some point, that multiple types where being sent. This just removes possible duplicates.
| .replace('${owner}', user) | ||
| .replace('${repo}', repo) | ||
| .replace('${number}', number) | ||
| const path = `${basePathFromTemplate}?per_page=100` |
There was a problem hiding this comment.
By default GitHub return the first 30 entries. 100 is the maximum. Also since query param is not described in the docs and doesn't work (hence filtering was written before, I assume).
There was a problem hiding this comment.
Overall the the added logic to support native pull request reviews seems to work well. However, it is important that there is a feature toggle both on the server side (globally allow/ignore native reviews) and from within the .zappr.yaml to configure the behavior per repository. When no extra configuration is added zappr should ignore native reviews and should not allow users to change this behavior per repository.
| @@ -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 | |||
| @@ -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 | |||
There was a problem hiding this comment.
When the default value pull_request = {} is applied, number is undefined. I a relatively huge function like this it is not obvious whether this is harmful or not.
| @@ -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? | |||
There was a problem hiding this comment.
Yes, looks like a c&p error.
|
@stoewer Thank you for the review. I'll try to comment/fix issues asap, but it might take longer than usual as I'm taking a long vacation soon. |
|
@GeorgySerga are you still working on this PR? if not maybe I can give it a shot? |
|
@louistsaitszho Hello! You're right, I've been lazy :) The code worked for me (counts native approvals and +1s), but in order to go through: need to make CR changes, write tests, add a toggle and update readme here, I think. If you want to contribute, sure thing, I can help as well, explaining some decisions, or setting things up (4 months passed, wow). |
|
3 more years passed. Is there any update on that? |
An attempt to fix: #544
This is a draft, work in progress. Code is working (expected to be working), no tests or toggle is implemented. More on the changes and reasoning would be inside inline comments. Thanks for reviews.