diff --git a/src/github/github-webhooks.service.spec.ts b/src/github/github-webhooks.service.spec.ts index cc4d331..6180009 100644 --- a/src/github/github-webhooks.service.spec.ts +++ b/src/github/github-webhooks.service.spec.ts @@ -136,6 +136,82 @@ describe('GithubWebhooksService', () => { expect(bountiesService.markMergedAndRelease).not.toHaveBeenCalled(); }); + it('deduplicates issue numbers in PR body and processes bounty once (#47)', async () => { + issueRepo.findOne.mockResolvedValue({ + id: 'issue-1', + bounty: { id: 'bounty-1' }, + }); + bountyRepo.findOne.mockResolvedValue({ id: 'bounty-1', status: 'claimed' }); + + const payload = { + action: 'closed', + number: 10, + pull_request: { + html_url: 'https://github.com/acme/repo/pull/10', + number: 10, + merged: true, + body: 'Fixes #12. This also resolves #12 as discussed in review.', + }, + repository: { id: 999, full_name: 'acme/repo' }, + }; + + const event = await service.handleEvent( + 'pull_request', + 'delivery-dedup', + payload, + true, + ); + + expect(bountiesService.markInReview).toHaveBeenCalledTimes(1); + expect(bountiesService.markMergedAndRelease).toHaveBeenCalledTimes(1); + expect(event.status).toBe(WebhookEventStatus.PROCESSED); + }); + + it('isolates per-issue errors and processes subsequent bounties when one fails (#47)', async () => { + issueRepo.findOne.mockImplementation(({ where }) => { + const num = where.number; + return Promise.resolve({ + id: `issue-${num}`, + bounty: { id: `bounty-${num}` }, + }); + }); + bountyRepo.findOne.mockImplementation(({ where }) => { + return Promise.resolve({ id: where.id, status: 'claimed' }); + }); + + bountiesService.markMergedAndRelease.mockImplementation((bountyId: string) => { + if (bountyId === 'bounty-12') { + return Promise.reject(new Error('escrow release failed')); + } + return Promise.resolve({ id: bountyId, status: 'paid' }); + }); + + const payload = { + action: 'closed', + number: 15, + pull_request: { + html_url: 'https://github.com/acme/repo/pull/15', + number: 15, + merged: true, + body: 'Fixes #12, Closes #34, Resolves #56', + }, + repository: { id: 999, full_name: 'acme/repo' }, + }; + + const event = await service.handleEvent( + 'pull_request', + 'delivery-multi', + payload, + true, + ); + + expect(bountiesService.markMergedAndRelease).toHaveBeenCalledWith('bounty-12'); + expect(bountiesService.markMergedAndRelease).toHaveBeenCalledWith('bounty-34'); + expect(bountiesService.markMergedAndRelease).toHaveBeenCalledWith('bounty-56'); + expect(event.status).toBe(WebhookEventStatus.FAILED); + expect(event.error).toContain('Issue #12: escrow release failed'); + }); + describe('"issues" webhook events (#24)', () => { const payload = { action: 'edited', diff --git a/src/github/github-webhooks.service.ts b/src/github/github-webhooks.service.ts index 20646b3..35bc5bb 100644 --- a/src/github/github-webhooks.service.ts +++ b/src/github/github-webhooks.service.ts @@ -111,40 +111,56 @@ export class GithubWebhooksService { return; } - const issueNumbers = this.extractLinkedIssueNumbers( + const rawIssueNumbers = this.extractLinkedIssueNumbers( payload.pull_request.body ?? '', ); - if (issueNumbers.length === 0) { + if (rawIssueNumbers.length === 0) { this.logger.warn( `PR #${payload.number} in ${payload.repository.full_name} merged but references no issue`, ); return; } + // Deduplicate issue numbers so PR bodies repeating the same issue don't trigger duplicate processing + const issueNumbers = [...new Set(rawIssueNumbers)]; + const errors: string[] = []; + for (const number of issueNumbers) { - const issue = await this.issueRepo.findOne({ - where: { - number, - repository: { githubRepoId: String(payload.repository.id) }, - }, - relations: { repository: true, bounty: true }, - }); - if (!issue?.bounty) continue; - - // Mark in_review first if it hadn't been (idempotent no-op if already there). - const bounty = await this.bountyRepo.findOne({ - where: { id: issue.bounty.id }, - }); - if (!bounty) continue; - - if (bounty.status === BountyStatus.CLAIMED) { - await this.bountiesService.markInReview( - bounty.id, - payload.pull_request.html_url, - payload.pull_request.number, + try { + const issue = await this.issueRepo.findOne({ + where: { + number, + repository: { githubRepoId: String(payload.repository.id) }, + }, + relations: { repository: true, bounty: true }, + }); + if (!issue?.bounty) continue; + + // Mark in_review first if it hadn't been (idempotent no-op if already there). + const bounty = await this.bountyRepo.findOne({ + where: { id: issue.bounty.id }, + }); + if (!bounty) continue; + + if (bounty.status === BountyStatus.CLAIMED) { + await this.bountiesService.markInReview( + bounty.id, + payload.pull_request.html_url, + payload.pull_request.number, + ); + } + await this.bountiesService.markMergedAndRelease(bounty.id); + } catch (err) { + const msg = `Issue #${number}: ${(err as Error).message}`; + this.logger.error( + `Failed processing linked bounty for ${msg} in PR #${payload.number}: ${(err as Error).stack}`, ); + errors.push(msg); } - await this.bountiesService.markMergedAndRelease(bounty.id); + } + + if (errors.length > 0) { + throw new Error(`Failed processing some linked bounties: ${errors.join('; ')}`); } }