Skip to content

Commit

Permalink
Resolve issue of allocating invalid assignees
Browse files Browse the repository at this point in the history
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:
CATcher-org#1264 (comment)
  • Loading branch information
woojiahao authored Mar 28, 2024
1 parent 5e7ed48 commit 62ef100
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 12 deletions.
65 changes: 55 additions & 10 deletions src/app/core/services/issue.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,17 +135,27 @@ export class IssueService {
}

updateIssue(issue: Issue): Observable<Issue> {
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<GithubIssue> {
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);
})
);
}
Expand Down Expand Up @@ -188,11 +198,17 @@ export class IssueService {
}

createTeamResponse(issue: Issue): Observable<Issue> {
// 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);
})
);
})
);
}
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 62ef100

Please sign in to comment.