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

@@ -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,

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

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

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<void> {
-    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<GithubIssue> {
     return from(octokit.issues.update({ owner: ORG_NAME, repo: REPO, issue_number: id, state: 'closed' })).pipe(
       map((response: GithubResponse<GithubIssue>) => {

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<Issue>());
   }
@@ -134,11 +136,6 @@ export class IssueService {
       .pipe(map((response: GithubIssue) => this.createIssueModel(response)));
   }
 
-  updateIssueWithAssigneeCheck(issue: Issue): Observable<Issue> {
-    const assignees = this.phaseService.currentPhase === Phase.phaseModeration ? [] : issue.assignees;
-    return this.githubService.areUsersAssignable(assignees).pipe(flatMap(() => this.updateIssue(issue)));
-  }
-
   updateIssue(issue: Issue): Observable<Issue> {
     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<Issue> {
     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);
+      })
     );
   }
 

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

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

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;

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

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

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