Skip to content

Commit 178240e

Browse files
NdpntNdpnt
authored
Reduce API calls and avoid inconsistent GitHub issues states (#1112)
2 parents 5cc709f + 9d20727 commit 178240e

File tree

3 files changed

+36
-119
lines changed

3 files changed

+36
-119
lines changed

‎CHANGELOG.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,14 @@
22

33
All changes that impact users of this module are documented in this file, in the [Common Changelog](https://common-changelog.org) format with some additional specifications defined in the CONTRIBUTING file. This codebase adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
44

5+
## Unreleased [patch]
6+
7+
> Development of this release was supported by the [French Ministry for Foreign Affairs](https://www.diplomatie.gouv.fr/fr/politique-etrangere-de-la-france/diplomatie-numerique/) through its ministerial [State Startups incubator](https://beta.gouv.fr/startups/open-terms-archive.html) under the aegis of the Ambassador for Digital Affairs.
8+
9+
### Fixed
10+
11+
- Reduce API calls and avoid inconsistent states on GitHub issues reports
12+
513
## 2.3.1 - 2024-10-22
614

715
_Full changeset and discussions: [#1111](https://github.com/OpenTermsArchive/engine/pull/1111)._

‎src/reporter/github.js‎

Lines changed: 18 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -79,28 +79,15 @@ export default class GitHub {
7979
return issue;
8080
}
8181

82-
async setIssueLabels({ issue, labels }) {
83-
await this.octokit.request('PUT /repos/{owner}/{repo}/issues/{issue_number}/labels', {
82+
async updateIssue(issue, { state, labels }) {
83+
const { data: updatedIssue } = await this.octokit.request('PATCH /repos/{owner}/{repo}/issues/{issue_number}', {
8484
...this.commonParams,
8585
issue_number: issue.number,
86+
state,
8687
labels,
8788
});
88-
}
8989

90-
async openIssue(issue) {
91-
await this.octokit.request('PATCH /repos/{owner}/{repo}/issues/{issue_number}', {
92-
...this.commonParams,
93-
issue_number: issue.number,
94-
state: GitHub.ISSUE_STATE_OPEN,
95-
});
96-
}
97-
98-
async closeIssue(issue) {
99-
await this.octokit.request('PATCH /repos/{owner}/{repo}/issues/{issue_number}', {
100-
...this.commonParams,
101-
issue_number: issue.number,
102-
state: GitHub.ISSUE_STATE_CLOSED,
103-
});
90+
return updatedIssue;
10491
}
10592

10693
async getIssue({ title, ...searchParams }) {
@@ -110,7 +97,7 @@ export default class GitHub {
11097
...searchParams,
11198
}, response => response.data);
11299

113-
const [issue] = issues.filter(item => item.title === title); // since only one is expected, use the first one
100+
const [issue] = issues.filter(item => item.title === title); // Since only one is expected, use the first one
114101

115102
return issue;
116103
}
@@ -134,9 +121,10 @@ export default class GitHub {
134121
}
135122

136123
await this.addCommentToIssue({ issue: openedIssue, comment });
137-
await this.closeIssue(openedIssue);
124+
logger.info(`Added comment to issue #${openedIssue.number}: ${openedIssue.html_url}`);
138125

139-
return logger.info(`Closed issue #${openedIssue.number}: ${openedIssue.html_url}`);
126+
await this.updateIssue(openedIssue, { state: GitHub.ISSUE_STATE_CLOSED });
127+
logger.info(`Closed issue #${openedIssue.number}: ${openedIssue.html_url}`);
140128
} catch (error) {
141129
logger.error(`Failed to update issue "${title}": ${error.message}`);
142130
}
@@ -152,23 +140,22 @@ export default class GitHub {
152140
return logger.info(`Created issue #${createdIssue.number} "${title}": ${createdIssue.html_url}`);
153141
}
154142

155-
if (issue.state == GitHub.ISSUE_STATE_CLOSED) {
156-
await this.openIssue(issue);
157-
logger.info(`Reopened issue #${issue.number}: ${issue.html_url}`);
158-
}
159-
160143
const managedLabelsNames = this.MANAGED_LABELS.map(label => label.name);
161-
const [managedLabel] = issue.labels.filter(label => managedLabelsNames.includes(label.name)); // it is assumed that only one specific reason for failure is possible at a time, making managed labels mutually exclusive
144+
const labelsNotManagedToKeep = issue.labels.map(label => label.name).filter(label => !managedLabelsNames.includes(label));
145+
const [managedLabel] = issue.labels.filter(label => managedLabelsNames.includes(label.name)); // It is assumed that only one specific reason for failure is possible at a time, making managed labels mutually exclusive
162146

163-
if (managedLabel?.name == label) { // if the label is already assigned to the issue, the error is redundant with the one already reported and no further action is necessary
147+
if (issue.state !== GitHub.ISSUE_STATE_CLOSED && managedLabel?.name === label) {
164148
return;
165149
}
166150

167-
const labelsNotManagedToKeep = issue.labels.map(label => label.name).filter(label => !managedLabelsNames.includes(label));
168-
169-
await this.setIssueLabels({ issue, labels: [ label, ...labelsNotManagedToKeep ] });
170-
await this.addCommentToIssue({ issue, comment: description });
151+
await this.updateIssue(issue, {
152+
state: GitHub.ISSUE_STATE_OPEN,
153+
labels: [ label, ...labelsNotManagedToKeep ],
154+
});
171155
logger.info(`Updated issue #${issue.number}: ${issue.html_url}`);
156+
await this.addCommentToIssue({ issue, comment: description });
157+
158+
logger.info(`Added comment to issue #${issue.number}: ${issue.html_url}`);
172159
} catch (error) {
173160
logger.error(`Failed to update issue "${title}": ${error.message}`);
174161
}

‎src/reporter/github.test.js‎

Lines changed: 10 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -118,66 +118,6 @@ describe('GitHub', function () {
118118
});
119119
});
120120

121-
describe('#setIssueLabels', () => {
122-
let scope;
123-
const ISSUE_NUMBER = 123;
124-
const LABELS = [ 'bug', 'enhancement' ];
125-
126-
before(async () => {
127-
scope = nock('https://api.github.com')
128-
.put(`/repos/owner/repo/issues/${ISSUE_NUMBER}/labels`, { labels: LABELS })
129-
.reply(200);
130-
131-
await github.setIssueLabels({ issue: { number: ISSUE_NUMBER }, labels: LABELS });
132-
});
133-
134-
after(nock.cleanAll);
135-
136-
it('sets labels on the issue', () => {
137-
expect(scope.isDone()).to.be.true;
138-
});
139-
});
140-
141-
describe('#openIssue', () => {
142-
let scope;
143-
const ISSUE = { number: 123 };
144-
const EXPECTED_REQUEST_BODY = { state: 'open' };
145-
146-
before(async () => {
147-
scope = nock('https://api.github.com')
148-
.patch(`/repos/owner/repo/issues/${ISSUE.number}`, EXPECTED_REQUEST_BODY)
149-
.reply(200);
150-
151-
await github.openIssue(ISSUE);
152-
});
153-
154-
after(nock.cleanAll);
155-
156-
it('opens the issue', () => {
157-
expect(scope.isDone()).to.be.true;
158-
});
159-
});
160-
161-
describe('#closeIssue', () => {
162-
let scope;
163-
const ISSUE = { number: 123 };
164-
const EXPECTED_REQUEST_BODY = { state: 'closed' };
165-
166-
before(async () => {
167-
scope = nock('https://api.github.com')
168-
.patch(`/repos/owner/repo/issues/${ISSUE.number}`, EXPECTED_REQUEST_BODY)
169-
.reply(200);
170-
171-
await github.closeIssue(ISSUE);
172-
});
173-
174-
after(nock.cleanAll);
175-
176-
it('closes the issue', () => {
177-
expect(scope.isDone()).to.be.true;
178-
});
179-
});
180-
181121
describe('#getIssue', () => {
182122
let scope;
183123
let result;
@@ -376,9 +316,8 @@ describe('GitHub', function () {
376316
};
377317

378318
context('when issue is closed', () => {
379-
let setIssueLabelsScope;
319+
let updateIssueScope;
380320
let addCommentScope;
381-
let openIssueScope;
382321

383322
const GITHUB_RESPONSE_FOR_EXISTING_ISSUE = {
384323
number: 123,
@@ -394,12 +333,8 @@ describe('GitHub', function () {
394333
.query(true)
395334
.reply(200, [GITHUB_RESPONSE_FOR_EXISTING_ISSUE]);
396335

397-
openIssueScope = nock('https://api.github.com')
398-
.patch(`/repos/owner/repo/issues/${GITHUB_RESPONSE_FOR_EXISTING_ISSUE.number}`, { state: GitHub.ISSUE_STATE_OPEN })
399-
.reply(200);
400-
401-
setIssueLabelsScope = nock('https://api.github.com')
402-
.put(`/repos/owner/repo/issues/${GITHUB_RESPONSE_FOR_EXISTING_ISSUE.number}/labels`, { labels: ['location'] })
336+
updateIssueScope = nock('https://api.github.com')
337+
.patch(`/repos/owner/repo/issues/${GITHUB_RESPONSE_FOR_EXISTING_ISSUE.number}`, { state: GitHub.ISSUE_STATE_OPEN, labels: ['location'] })
403338
.reply(200);
404339

405340
addCommentScope = nock('https://api.github.com')
@@ -409,12 +344,8 @@ describe('GitHub', function () {
409344
await github.createOrUpdateIssue(ISSUE);
410345
});
411346

412-
it('reopens the issue', () => {
413-
expect(openIssueScope.isDone()).to.be.true;
414-
});
415-
416-
it("updates the issue's label", () => {
417-
expect(setIssueLabelsScope.isDone()).to.be.true;
347+
it('reopens the issue and its labels', () => {
348+
expect(updateIssueScope.isDone()).to.be.true;
418349
});
419350

420351
it('adds comment to the issue', () => {
@@ -423,9 +354,8 @@ describe('GitHub', function () {
423354
});
424355

425356
context('when issue is already opened', () => {
426-
let setIssueLabelsScope;
427357
let addCommentScope;
428-
let openIssueScope;
358+
let updateIssueScope;
429359

430360
const GITHUB_RESPONSE_FOR_EXISTING_ISSUE = {
431361
number: 123,
@@ -441,12 +371,8 @@ describe('GitHub', function () {
441371
.query(true)
442372
.reply(200, [GITHUB_RESPONSE_FOR_EXISTING_ISSUE]);
443373

444-
openIssueScope = nock('https://api.github.com')
445-
.patch(`/repos/owner/repo/issues/${GITHUB_RESPONSE_FOR_EXISTING_ISSUE.number}`, { state: GitHub.ISSUE_STATE_OPEN })
446-
.reply(200);
447-
448-
setIssueLabelsScope = nock('https://api.github.com')
449-
.put(`/repos/owner/repo/issues/${GITHUB_RESPONSE_FOR_EXISTING_ISSUE.number}/labels`, { labels: ['location'] })
374+
updateIssueScope = nock('https://api.github.com')
375+
.patch(`/repos/owner/repo/issues/${GITHUB_RESPONSE_FOR_EXISTING_ISSUE.number}`, { state: GitHub.ISSUE_STATE_OPEN, labels: ['location'] })
450376
.reply(200);
451377

452378
addCommentScope = nock('https://api.github.com')
@@ -456,12 +382,8 @@ describe('GitHub', function () {
456382
await github.createOrUpdateIssue(ISSUE);
457383
});
458384

459-
it('does not change the issue state', () => {
460-
expect(openIssueScope.isDone()).to.be.false;
461-
});
462-
463-
it("updates the issue's label", () => {
464-
expect(setIssueLabelsScope.isDone()).to.be.true;
385+
it("updates the issue's labels", () => {
386+
expect(updateIssueScope.isDone()).to.be.true;
465387
});
466388

467389
it('adds comment to the issue', () => {

0 commit comments

Comments
 (0)