From 62ef100677a765c2d19ab65b255875ff3ca6b45b Mon Sep 17 00:00:00 2001 From: Jiahao Date: Thu, 28 Mar 2024 12:57:33 +0800 Subject: [PATCH] Resolve issue of allocating invalid assignees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, when a user attempts to submit a team response with an invalid assignee (defined as an assignee who has not joined the organization), the team response is processed (by adding the team response GitHub comment) before the assignee checks are made. The error message for an invalid assignee is also vague, only stating "Validation Failed", with no additional context. This causes the issue to be erroneously classified as “Responded” when it has not actually been properly responded to as the “assignee” field would be left empty. The vague invalid assignee message also means users will not know what to do to fix the issue. This issue can be resolved by inverting the logic of team responses, ensuring the assignee validity is checked before making the team response comment. If the assignee is invalid, then the team response comment is never created, thus avoiding the erroneous classification. Also provided custom error parsing for invalid assignees to display proper reasons and actions for the validation failure for users to rectify the issue. A proper diagram of the inversion of logic can be found in the pull request: https://github.com/CATcher-org/CATcher/pull/1264#issuecomment-2022201156 --- src/app/core/services/issue.service.ts | 65 ++++++++++++++++--- .../new-team-response.component.ts | 4 +- 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/src/app/core/services/issue.service.ts b/src/app/core/services/issue.service.ts index 5b55cab15..a7afb2f58 100644 --- a/src/app/core/services/issue.service.ts +++ b/src/app/core/services/issue.service.ts @@ -135,17 +135,27 @@ export class IssueService { } updateIssue(issue: Issue): Observable { + return this.updateGithubIssue(issue).pipe( + map((githubIssue: GithubIssue) => { + githubIssue.comments = issue.githubComments; + return this.createIssueModel(githubIssue); + }) + ); + } + + /** + * Updates an issue without attempting to create an issue model. Used when we want to treat + * updateIssue as an atomic operation that only performs an API call. + * @param issue current issue model + * @returns GitHubIssue from the API request + */ + updateGithubIssue(issue: Issue): Observable { const assignees = this.phaseService.currentPhase === Phase.phaseModeration ? [] : issue.assignees; return this.githubService .updateIssue(issue.id, issue.title, this.createGithubIssueDescription(issue), this.createLabelsForIssue(issue), assignees) .pipe( - map((response: GithubIssue) => { - response.comments = issue.githubComments; - return this.createIssueModel(response); - }), catchError((err) => { - this.logger.error('IssueService: ', err); // Log full details of error first - return throwError(err.response.data.message); // More readable error message + return this.parseUpdateIssueResponseError(err); }) ); } @@ -188,11 +198,17 @@ export class IssueService { } createTeamResponse(issue: Issue): Observable { + // The issue must be updated first to ensure that fields like assignees are valid const teamResponse = issue.createGithubTeamResponse(); - return this.githubService.createIssueComment(issue.id, teamResponse).pipe( - mergeMap((githubComment: GithubComment) => { - issue.githubComments = [githubComment, ...issue.githubComments.filter((c) => c.id !== githubComment.id)]; - return this.updateIssue(issue); + return this.updateGithubIssue(issue).pipe( + mergeMap((response: GithubIssue) => { + return this.githubService.createIssueComment(issue.id, teamResponse).pipe( + map((githubComment: GithubComment) => { + issue.githubComments = [githubComment, ...issue.githubComments.filter((c) => c.id !== githubComment.id)]; + response.comments = issue.githubComments; + return this.createIssueModel(response); + }) + ); }) ); } @@ -479,6 +495,35 @@ export class IssueService { return issue; } + private parseUpdateIssueResponseError(err: any) { + this.logger.error('IssueService: ', err); // Log full details of error first + + if (err.code !== 422 || !err.hasOwnProperty('message')) { + return throwError(err.response.data.message); // More readable error message + } + + // Error code 422 implies that one of the fields are invalid + const validationFailedPrefix = 'Validation Failed:'; + const message: string = err.message; + const errorJsonRaw = message.substring(validationFailedPrefix.length); + const errorJson = JSON.parse(errorJsonRaw); + + const mandatoryFields = ['field', 'code', 'value']; + const hasMandatoryFields = mandatoryFields.every((field) => errorJson.hasOwnProperty(field)); + + if (hasMandatoryFields) { + if (errorJson['field'] === 'assignees' && errorJson['code'] === 'invalid') { + // If assignees are invalid, return a custom error + return throwError( + `Assignee ${errorJson['value']} has not joined your organization yet. Please remove them from the assignees list.` + ); + } + } + + // Generic 422 Validation Failed since it is not an assignees problem + return throwError(err.response.data.message); + } + setIssueTeamFilter(filterValue: string) { if (filterValue) { this.issueTeamFilter = filterValue; diff --git a/src/app/shared/view-issue/new-team-response/new-team-response.component.ts b/src/app/shared/view-issue/new-team-response/new-team-response.component.ts index 7c963e46c..fb3b7add6 100644 --- a/src/app/shared/view-issue/new-team-response/new-team-response.component.ts +++ b/src/app/shared/view-issue/new-team-response/new-team-response.component.ts @@ -108,9 +108,9 @@ export class NewTeamResponseComponent implements OnInit, OnDestroy { this.isSafeToSubmit() .pipe( - mergeMap((isSaveToSubmit: boolean) => { + mergeMap((isSafeToSubmit: boolean) => { const newCommentDescription = latestIssue.createGithubTeamResponse(); - if (isSaveToSubmit) { + if (isSafeToSubmit) { return this.issueService.createTeamResponse(latestIssue); } else if (this.submitButtonText === SUBMIT_BUTTON_TEXT.OVERWRITE) { const issueCommentId = this.issueService.issues[this.issue.id].issueComment.id;