From b8a5a34ecfebfcbeab4b831964b6bf834bd9ea88 Mon Sep 17 00:00:00 2001 From: Lee Chun Wei <47494777+chunweii@users.noreply.github.com> Date: Sat, 23 Apr 2022 18:14:37 +0800 Subject: [PATCH] Remove assignee check API (#933) In a previous PR, there was a buggy implementation of checking whether assignees are authorized to be assigned due to lack of pagination. It makes the code unnecessarily complex. Let's remove the check altogether and just display the "Validation Failed" message from GitHub to the user as it is. --- src/app/app.module.ts | 2 +- .../core/services/error-handling.service.ts | 2 ++ .../factories/factory.issue.service.ts | 6 ++-- src/app/core/services/github.service.ts | 22 --------------- src/app/core/services/issue.service.ts | 28 ++++++++----------- .../issue/assignee/assignee.component.ts | 2 +- .../issues-faulty.component.spec.ts | 2 +- .../issues-pending.component.spec.ts | 2 +- .../issues-responded.component.spec.ts | 2 +- .../shared/issue-tables/search-filter.spec.ts | 2 +- .../issue/assignee/assignee.component.spec.ts | 11 ++------ 11 files changed, 27 insertions(+), 54 deletions(-) diff --git a/src/app/app.module.ts b/src/app/app.module.ts index 59a5fde75..a90d87bf8 100644 --- a/src/app/app.module.ts +++ b/src/app/app.module.ts @@ -87,7 +87,7 @@ import { SharedModule } from './shared/shared.module'; { provide: IssueService, useFactory: IssueServiceFactory, - deps: [GithubService, UserService, PhaseService, ElectronService, DataService] + deps: [GithubService, UserService, PhaseService, ElectronService, DataService, LoggingService] }, { provide: ErrorHandler, diff --git a/src/app/core/services/error-handling.service.ts b/src/app/core/services/error-handling.service.ts index ea27b5669..b38eec07b 100644 --- a/src/app/core/services/error-handling.service.ts +++ b/src/app/core/services/error-handling.service.ts @@ -25,6 +25,8 @@ export class ErrorHandlingService implements ErrorHandler { this.handleHttpError(error, actionCallback); } else if (error.constructor.name === 'RequestError') { this.handleHttpError(error as RequestError, actionCallback); + } else if (typeof error === 'string') { + this.handleGeneralError(error); } else { this.handleGeneralError(error.message || JSON.stringify(error)); } diff --git a/src/app/core/services/factories/factory.issue.service.ts b/src/app/core/services/factories/factory.issue.service.ts index f167da027..ea67a2271 100644 --- a/src/app/core/services/factories/factory.issue.service.ts +++ b/src/app/core/services/factories/factory.issue.service.ts @@ -3,6 +3,7 @@ import { DataService } from '../data.service'; import { ElectronService } from '../electron.service'; import { GithubService } from '../github.service'; import { IssueService } from '../issue.service'; +import { LoggingService } from '../logging.service'; import { MockIssueService } from '../mocks/mock.issue.service'; import { PhaseService } from '../phase.service'; import { UserService } from '../user.service'; @@ -12,10 +13,11 @@ export function IssueServiceFactory( userService: UserService, phaseService: PhaseService, electronService: ElectronService, - dataService: DataService + dataService: DataService, + logger: LoggingService ) { if (AppConfig.test) { return new MockIssueService(githubService, phaseService, dataService); } - return new IssueService(githubService, userService, phaseService, electronService, dataService); + return new IssueService(githubService, userService, phaseService, electronService, dataService, logger); } diff --git a/src/app/core/services/github.service.ts b/src/app/core/services/github.service.ts index 0916fab54..ef390614f 100644 --- a/src/app/core/services/github.service.ts +++ b/src/app/core/services/github.service.ts @@ -259,28 +259,6 @@ export class GithubService { octokit.issues.updateLabel({ owner: ORG_NAME, repo: REPO, name: labelName, current_name: labelName, color: labelColor }); } - /** - * Checks if the given list of users are allowed to be assigned to an issue. - * @param assignees - GitHub usernames to be checked - */ - areUsersAssignable(assignees: string[]): Observable { - return from( - octokit.issues.listAssignees({ - owner: ORG_NAME, - repo: REPO - }) - ).pipe( - map(({ data }: { data: { login: string }[] }) => data.map(({ login }) => login)), - map((assignables: string[]) => - assignees.forEach((assignee) => { - if (!assignables.includes(assignee)) { - throw new Error(`Cannot assign ${assignee} to the issue. Please check if ${assignee} is authorized.`); - } - }) - ) - ); - } - closeIssue(id: number): Observable { return from(octokit.issues.update({ owner: ORG_NAME, repo: REPO, issue_number: id, state: 'closed' })).pipe( map((response: GithubResponse) => { diff --git a/src/app/core/services/issue.service.ts b/src/app/core/services/issue.service.ts index 7671d1df7..bdad5e1bf 100644 --- a/src/app/core/services/issue.service.ts +++ b/src/app/core/services/issue.service.ts @@ -14,6 +14,7 @@ import { appVersion } from './application.service'; import { DataService } from './data.service'; import { ElectronService } from './electron.service'; import { GithubService } from './github.service'; +import { LoggingService } from './logging.service'; import { PhaseService } from './phase.service'; import { UserService } from './user.service'; @@ -42,7 +43,8 @@ export class IssueService { private userService: UserService, private phaseService: PhaseService, private electronService: ElectronService, - private dataService: DataService + private dataService: DataService, + private logger: LoggingService ) { this.issues$ = new BehaviorSubject(new Array()); } @@ -134,11 +136,6 @@ export class IssueService { .pipe(map((response: GithubIssue) => this.createIssueModel(response))); } - updateIssueWithAssigneeCheck(issue: Issue): Observable { - const assignees = this.phaseService.currentPhase === Phase.phaseModeration ? [] : issue.assignees; - return this.githubService.areUsersAssignable(assignees).pipe(flatMap(() => this.updateIssue(issue))); - } - updateIssue(issue: Issue): Observable { const assignees = this.phaseService.currentPhase === Phase.phaseModeration ? [] : issue.assignees; return this.githubService @@ -147,6 +144,10 @@ export class IssueService { map((response: GithubIssue) => { response.comments = issue.githubComments; return this.createIssueModel(response); + }), + catchError((err) => { + this.logger.error(err); // Log full details of error first + return throwError(err.response.data.message); // More readable error message }) ); } @@ -190,16 +191,11 @@ export class IssueService { createTeamResponse(issue: Issue): Observable { const teamResponse = issue.createGithubTeamResponse(); - return this.githubService.areUsersAssignable(issue.assignees || []).pipe( - flatMap(() => - this.githubService.createIssueComment(issue.id, teamResponse).pipe( - flatMap((githubComment: GithubComment) => { - issue.githubComments = [githubComment, ...issue.githubComments.filter((c) => c.id !== githubComment.id)]; - return this.updateIssue(issue); - }) - ) - ), - catchError((err) => throwError(err)) + return this.githubService.createIssueComment(issue.id, teamResponse).pipe( + flatMap((githubComment: GithubComment) => { + issue.githubComments = [githubComment, ...issue.githubComments.filter((c) => c.id !== githubComment.id)]; + return this.updateIssue(issue); + }) ); } diff --git a/src/app/shared/issue/assignee/assignee.component.ts b/src/app/shared/issue/assignee/assignee.component.ts index 0e20fb74f..c56de4ecf 100644 --- a/src/app/shared/issue/assignee/assignee.component.ts +++ b/src/app/shared/issue/assignee/assignee.component.ts @@ -54,7 +54,7 @@ export class AssigneeComponent implements OnInit { const newIssue = this.issue.clone(this.phaseService.currentPhase); const oldAssignees = newIssue.assignees; newIssue.assignees = this.assignees; - this.issueService.updateIssueWithAssigneeCheck(newIssue).subscribe( + this.issueService.updateIssue(newIssue).subscribe( (updatedIssue: Issue) => { this.issueUpdated.emit(updatedIssue); // Update assignees of duplicate issues diff --git a/tests/app/phase-team-response/issues-faulty/issues-faulty.component.spec.ts b/tests/app/phase-team-response/issues-faulty/issues-faulty.component.spec.ts index 9664ca9d5..83381a1a2 100644 --- a/tests/app/phase-team-response/issues-faulty/issues-faulty.component.spec.ts +++ b/tests/app/phase-team-response/issues-faulty/issues-faulty.component.spec.ts @@ -17,7 +17,7 @@ describe('IssuesFaultyComponent', () => { const DUMMY_RESPONSE = 'dummy response'; beforeEach(() => { - issueService = new IssueService(null, null, null, null, null); + issueService = new IssueService(null, null, null, null, null, null); issueService.updateLocalStore(dummyIssue); issuesFaultyComponent = new IssuesFaultyComponent(issueService, userService, null); issuesFaultyComponent.ngOnInit(); diff --git a/tests/app/phase-team-response/issues-pending/issues-pending.component.spec.ts b/tests/app/phase-team-response/issues-pending/issues-pending.component.spec.ts index 22df4e78d..b99657927 100644 --- a/tests/app/phase-team-response/issues-pending/issues-pending.component.spec.ts +++ b/tests/app/phase-team-response/issues-pending/issues-pending.component.spec.ts @@ -11,7 +11,7 @@ describe('IssuesPendingComponent', () => { const dummyTeam: Team = TEAM_4; let dummyIssue: Issue; let issuesPendingComponent: IssuesPendingComponent; - const issueService: IssueService = new IssueService(null, null, null, null, null); + const issueService: IssueService = new IssueService(null, null, null, null, null, null); const userService: UserService = new UserService(null, null); userService.currentUser = USER_Q; const DUMMY_DUPLICATE_ISSUE_ID = 1; diff --git a/tests/app/phase-team-response/issues-responded/issues-responded.component.spec.ts b/tests/app/phase-team-response/issues-responded/issues-responded.component.spec.ts index dcda46eac..1b6d18802 100644 --- a/tests/app/phase-team-response/issues-responded/issues-responded.component.spec.ts +++ b/tests/app/phase-team-response/issues-responded/issues-responded.component.spec.ts @@ -12,7 +12,7 @@ describe('IssuesRespondedComponent', () => { const DUMMY_RESPONSE = 'dummy response'; let dummyIssue: Issue; - const issueService = new IssueService(null, null, null, null, null); + const issueService = new IssueService(null, null, null, null, null, null); const userService = new UserService(null, null); userService.currentUser = USER_Q; const issuesRespondedComponent = new IssuesRespondedComponent(issueService, userService); diff --git a/tests/app/shared/issue-tables/search-filter.spec.ts b/tests/app/shared/issue-tables/search-filter.spec.ts index 0a03446c7..e51fdbc39 100644 --- a/tests/app/shared/issue-tables/search-filter.spec.ts +++ b/tests/app/shared/issue-tables/search-filter.spec.ts @@ -45,7 +45,7 @@ describe('search-filter', () => { TABLE_COLUMNS.ASSIGNEE, TABLE_COLUMNS.DUPLICATED_ISSUES ]; - const issueService: IssueService = new IssueService(null, null, null, null, null); + const issueService: IssueService = new IssueService(null, null, null, null, null, null); beforeEach(() => { issueService.updateLocalStore(mediumSeverityIssueWithResponse); diff --git a/tests/app/shared/issue/assignee/assignee.component.spec.ts b/tests/app/shared/issue/assignee/assignee.component.spec.ts index 05437c6d9..7c89a20fc 100644 --- a/tests/app/shared/issue/assignee/assignee.component.spec.ts +++ b/tests/app/shared/issue/assignee/assignee.component.spec.ts @@ -51,12 +51,7 @@ describe('AssigneeComponent', () => { }); const phaseService: any = jasmine.createSpyObj('PhaseService', [], { currentPhase: Phase.phaseTeamResponse }); - const issueService: any = jasmine.createSpyObj('IssueService', [ - 'getDuplicateIssuesFor', - 'getLatestIssue', - 'updateIssue', - 'updateIssueWithAssigneeCheck' - ]); + const issueService: any = jasmine.createSpyObj('IssueService', ['getDuplicateIssuesFor', 'getLatestIssue', 'updateIssue']); const permissionsService: any = jasmine.createSpyObj('PermissionService', ['isIssueLabelsEditable']); beforeEach(async(() => { @@ -131,7 +126,7 @@ describe('AssigneeComponent', () => { const updatedDuplicateIssue = duplicateIssue.clone(phaseService.currentPhase); updatedDuplicateIssue.assignees = [testStudent.loginId]; - expect(issueService.updateIssueWithAssigneeCheck).toHaveBeenCalledWith(updatedIssue); + expect(issueService.updateIssue).toHaveBeenCalledWith(updatedIssue); expect(issueService.updateIssue).toHaveBeenCalledWith(updatedDuplicateIssue); }); @@ -149,7 +144,7 @@ describe('AssigneeComponent', () => { function dispatchClosedEvent() { const matSelectElement: HTMLElement = debugElement.query(By.css('.mat-select')).nativeElement; - issueService.updateIssueWithAssigneeCheck.and.callFake((updatedIssue: Issue) => of(updatedIssue)); + issueService.updateIssue.and.callFake((updatedIssue: Issue) => of(updatedIssue)); matSelectElement.dispatchEvent(new Event('closed')); fixture.detectChanges(); }