Submitting a response results in invalid severity while status is responded

[woojiahao]

Describe the bug Note: this bug was triggered while performing CATcher testing for CS2103/T this semester.

When filling in an issue response, there are times where the submission causes a "Validation Failed" error. Clicking the submit button another time triggers a submission but the following screen appears when viewing the details of the response.

Subsequently, these issues are flagged as responded but when viewing it on the main page, we see the following error in the response column:

To Reproduce Steps to reproduce the behavior:

1. Login to CATcher
2. Select an issue to respond to
3. Select the last person in the list of assignees only
4. Select the "Rejected" status
5. Fill in the text body
6. Submit the response
7. If "Validation Failed", re-submit the response
8. Try viewing the details of the response
9. Return to main page and view response overview

Expected behavior Submitted response should not have a missing "Response" status value

Screenshots See description

Desktop (please complete the following information):

• OS: MacOS Sonoma v14.3.1
• Browser: Firefox
• Version: 124.0

Additional context N/A

[woojiahao]

I was going to file this bug report last week but had other work to attend to before that.

Was looking through the code and setting up the repository to replicate this but in the meantime, if this issue is resolved, do let me know

[damithc]

@woojiahao I figured out the cause. The person you are trying to assign has not joined the GitHub organization. That's why CATcher runs into problems when trying to assign him to a bug.

[woojiahao]

Ah, no wonder I was not able to replicate it on my end either 🤔 , I had already joined the repository with my alternate account. Thank you @damithc !!

This seems like something that should be enforced prior to the team response phase? It seems like it's less of an issue during the actual PE because students are required to have joined during the bug reporting phase but nevertheless, an issue that should be handled to avoid future reports of such a bug.

From my very rough dive through the codebase, seems like an issue that would have two potential fixes:

1. Block selection of assignees who have not joined the organization in the assignee drop down, affecting the /src/app/shared/issue/assignee/assignee.component.ts component and /src/app/shared/issue/assignee/assignee.component.html layout file where we should be adding an additional check prior to populating the teamMembers field:

    <mat-option *ngFor="let assignee of teamMembers" [value]="assignee">{{ assignee }}</mat-option>

And

  ngOnInit(): void {
    // This could be improved
    this.teamMembers = this.team.teamMembers.map((user) => user.loginId);
  }

I think this approach is good because it gives a more concrete way to reach out to those who aren't joined to finally join since their teammates can probe them about it.

2. Perform a one-time sweep through the system to flag out the users who have not joined the organization. However, this might not result in users joining the organization either.

From a UX perspective, I feel (1) is likely going to result in more "action" from those who haven't joined since their friends/teammates will push them to do it. But I'm not entirely sure what the team has in mind for this so please do advise on this :)

[woojiahao]

Ah I see an issue with (1) and that is the user data stored is directly from data.csv so doing the check would require a bulk query anyways.

The alternative I'm thinking of is to just flag a proper error and unselecting the assignee. Am currently working on a POC for this

[woojiahao]

From GitHub's API, 422 is thrown which indicates an unprocessable entity (i.e. user is not in organization). It's probably good to catch these specific errors to notify the user of the error

This being the full message body:

message: 'Validation Failed: {"value":"programmingperspective","resource":"Issue","field":"assignees","code":"invalid"}'

[woojiahao]

In a rough draft, this is what it could look like over the generic "Validation Failed" error:

This is triggered the moment the "Assignee" dropdown is closed or submission is done.

For the code, the only place that would require modification is issue.service.ts where we just need to parse the error body before throwing the error:

updateIssue(issue: Issue): Observable<Issue> {
  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

        if (err.code !== 422 || !err.hasOwnProperty('message')) {
          return throwError(err.response.data.message); // More readable error message
        }

        const validationFailedPrefix = 'Validation Failed:';
        const message: string = err.message;
        const errorJsonRaw = message.substring(validationFailedPrefix.length);
        const errorJson = JSON.parse(errorJsonRaw);

        if (!(errorJson.hasOwnProperty('field')
          && errorJson.hasOwnProperty('code')
          && errorJson.hasOwnProperty('value'))) {
          return throwError(err.response.data.message); // More readable error message
        }

        // Verify if assignee is the issue
        if (errorJson['field'] === 'assignees' && errorJson['code'] === 'invalid') {
          return throwError(`Assignee ${errorJson['value']} has not joined your organization yet.`);
        }
      })
    );
}

But once again, I'm not entirely sure what the CATcher team's stance on this is, just thought it was an interesting issue that I wanted to look into :>

[damithc]

@woojiahao Thanks a lot for working on this. Yes, as you mentioned, this is unlikely to happen during the PE but it is always better to be prepared. I think giving a more informative error message is a good way to handle this case. The error message you suggested is good. Perhaps add and therefore, cannot be assigned to a bug report.?

[woojiahao]

@damithc No worries, it is an interesting issue that arose.

Perhaps add and therefore, cannot be assigned to a bug report.?

Yup! That will work :)

Additionally, would it make sense to clear the assignee selection to avoid false submissions? Asking since that may require altering the the return values of the updateIssue catchError to make this distinction on the front-end.

[damithc]

Additionally, would it make sense to clear the assignee selection to avoid false submissions? Asking since that may require altering the the return values of the updateIssue catchError to make this distinction on the front-end.

@woojiahao The alternative is to mention this in the error message instead (e.g., please remove abc from the selected assignees ...), if the resulting code is simpler. As this case is unlikely to happen in practice (i.e., low payback), it's better if the cost of the fix is kept minimal as well.

BTW, this submission should not go through no matter how many times the user tries it. Earlier you mentioned that it went through in the second try?

[woojiahao]

As this case is unlikely to happen in practice (i.e., low payback), it's better if the cost of the fix is kept minimal as well.

Sounds good!

Earlier you mentioned that it went through in the second try?

Yup! So this is another issue that I'm looking into as well

[woojiahao]

Digging around the code reveals that the first submission, despite being a failure, still submits the comment to GitHub. So in fact, the submission works the first time. The second submission just triggers a visual change

[woojiahao]

I found the issue in issue.service.ts

  createTeamResponse(issue: Issue): Observable<Issue> {
    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);
      })
    );
  }

Even if updateIssue (which updates the metadata of the issue) fails, the comment is still added which is not the intended outcome. The fix for this should be relatively straightforward but I'll have to figure out Rx.js first haha

From first impressions, it seems that the logic should be inverted, so updating the issue's metadata should come first to detect the assignee issues, and only when it succeeds, then we should update the issue comment

[woojiahao]

Also noticed a minor typo in the code for new-team-response.component.ts :p

    this.isSafeToSubmit()
      .pipe(
        mergeMap((isSaveToSubmit: boolean) => {
          const newCommentDescription = latestIssue.createGithubTeamResponse();
          if (isSaveToSubmit) {

It should be isSafetoSubmit, not isSaveToSubmit

I'll fix these when PR-ing aha

[woojiahao]

I think the inversion of logic should be correct:

  createTeamResponse(issue: Issue): Observable<Issue> {
    const teamResponse = issue.createGithubTeamResponse();
    return this.updateIssue(issue).pipe(
      map((updatedIssue: Issue) => {
        this.githubService.createIssueComment(updatedIssue.id, teamResponse);
        return updatedIssue;
      })
    )
  }

So we first attempt to update the issue first before we make the comment to ensure that any errors are logged and caught to avoid such cases.

I'll clean up the code tomorrow morning and make a proper PR then :)

[woojiahao]

@damithc I've opened a PR for this issue, had to create a "pure" version of updateIssue (reasons for that listed in the PR)

[damithc]

@damithc I've opened a PR for this issue, had to create a "pure" version of updateIssue (reasons for that listed in the PR)

Great. Thanks for the help @woojiahao 💯