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(); }