Skip to content

Commit

Permalink
Drop the user check from the github comment/inline comment lookups
Browse files Browse the repository at this point in the history
  • Loading branch information
orta committed Apr 16, 2024
1 parent fee8473 commit 9e8d1c6
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 11 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -23,6 +23,7 @@ Bumping to 12.x because we've raised the minimum to node version from 14 to 18.
requiring a newer version of node. This is a breaking change for some folk! Also, 14 has been out of support for quite a while
now and Node 18 gives us a full year. - [@orta]

- Remove the user checks in GitHub comment/inline comment lookups, to allow using app tokens [#1433] - [@orta]
- Upgrade `node` engine from `>=14.13.1` to `>=18` [@heltoft]
- Upgrade `@types/node` from `^10.11.3` to `18.19.18` [@heltoft]
- GitLab: [#1386] Move from `@gitbeaker/node` to `@gitbeaker/rest` [@heltoft]
Expand Down
9 changes: 3 additions & 6 deletions source/platforms/github/GitHubAPI.ts
Expand Up @@ -83,14 +83,11 @@ export class GitHubAPI {
// The above is the API for Platform

getDangerCommentIDs = async (dangerID: string): Promise<string[]> => {
const userID = await this.getUserID()
const allComments = await this.getPullRequestComments()
const dangerIDMessage = dangerIDToString(dangerID)
this.d(`User ID: ${userID}`)
this.d(`Looking at ${allComments.length} comments for ${dangerIDMessage}`)
return allComments
.filter((comment) => comment.body.includes(dangerIDMessage)) // does it contain the right danger ID?
.filter((comment) => comment.user.id === userID) // Does it have the right user ID?
.filter((comment) => comment.body.includes("Generated by")) // Does it look like a danger message?
.map((comment) => comment.id) // only return IDs
}
Expand Down Expand Up @@ -318,15 +315,15 @@ export class GitHubAPI {
getPullRequestInlineComments = async (
dangerID: string
): Promise<(GitHubIssueComment & { ownedByDanger: boolean })[]> => {
const userID = await this.getUserID()
const repo = this.repoMetadata.repoSlug
const prID = this.repoMetadata.pullRequestID
const dangerIDMessage = dangerIDToString(dangerID)
return await this.getAllOfResource(`repos/${repo}/pulls/${prID}/comments`).then((v: GitHubIssueComment[]) => {
return v
.filter(Boolean)
.filter((i) => !!i)
.map((i) => {
return { ...i, ownedByDanger: i.user.id == userID && i.body.includes(dangerIDMessage) }
// Can't use && i.body.includes("Generated by") because it's not there on an inline comment
return { ...i, ownedByDanger: i.body.includes(dangerIDMessage) }
})
.filter((i) => i.ownedByDanger)
})
Expand Down
8 changes: 3 additions & 5 deletions source/platforms/github/_tests/_github_api.test.ts
Expand Up @@ -108,21 +108,19 @@ describe("API testing", () => {
expect(commentIDs.length).toEqual(0)
})

it("getPullRequestInlineComment gets only comments for given DangerId", async () => {
it("getPullRequestInlineComment gets only comments for given dangerID", async () => {
api.getAllOfResource = await requestWithFixturedJSON("github_inline_comments_with_danger.json")
api.getUserID = () => new Promise<number>((r) => r(20229914))

const comments = await api.getPullRequestInlineComments("default")

expect(comments.length).toEqual(1)
expect(comments[0].ownedByDanger).toBeTruthy()
})

it("getPullRequestInlineComment doesn't get comments as the DangerId is different", async () => {
it("getPullRequestInlineComment doesn't get comments as the dangerID is different", async () => {
api.getAllOfResource = await requestWithFixturedJSON("github_inline_comments_with_danger.json")
api.getUserID = () => new Promise<number>((r) => r(123))

const comments = await api.getPullRequestInlineComments("default")
const comments = await api.getPullRequestInlineComments("other-danger-id")

expect(comments.length).toEqual(0)
})
Expand Down

0 comments on commit 9e8d1c6

Please sign in to comment.