Raising warnings when submitting team response with assignees who aren't in organization

woojiahao commented 3 months ago

Summary:

Fixes #1262

Changes Made:

Custom parsing of updateIssue errors to detect when assignees are invalid and providing custom error messages
Inverts the flow of creating a team response (current flow still creates the comment even if the assignee information is wrong, but this causes the issue to be falsely flagged as "responded"

Proposed Commit Message:

Resolve issue of allocating invalid assignees

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

woojiahao commented 3 months ago

For the second fix, the primary issue is that the comment is added before the assignee issue could be detected which falsely sets the issue to be "responded" when the assignee field is actually blank.

Inverting the order would resolve this issue logically, but the current updateIssue is dependent on the updated GitHub comments which means that the inherent behavior is tied to the old ordering (create comment -> assign issue metadata) than the new one.

Effectively, the current updateIssue method isn't pure enough to treat it as an atomic operation as it attempts to create an Issue model as well.

I'm currently investigating ways to fix this issue, one way is to create a "shadow" copy of updateIssue that does not perform the automatic parsing from GitHubIssue -> Issue until the final operation is completed.

woojiahao commented 3 months ago

For the sake of clarity, the following is the original flow of creating a team response

sequenceDiagram
  participant Caller
  participant i as IssueService
  participant g as GitHubService
  Caller ->> i : createTeamResponse(Issue)
  i ->> g : createIssueComment
  g -->> i : GitHubComment
  i ->> i : Update Issue githubComments field with latest comments
  i ->> i : updateIssue
  i ->> g : updateIssue
  alt valid fields
  g -->> i : GitHubIssue
  i ->> i : createIssueModel(GitHubIssue)
  else else
  g -->> i : Error
  i ->> i : Parse error
  end
  i -->> Caller : Complete

The new flow proposed does the following

sequenceDiagram
  participant Caller
  participant i as IssueService
  participant g as GitHubService
  Caller ->> i : createTeamResponse(Issue)
  i ->> i : updateIssuePure
  i ->> g : updateIssue
  alt valid fields
  g -->> i : GitHubIssue
  i ->> g : createIssueComment
  g -->> i : GitHubComment
  i ->> i : Update Issue githubComments field with latest comments
  i ->> i : createIssueModel(GitHubIssue)
  i -->> Caller : Complete
  else else
  g -->> i : Error
  i ->> i : Parse error
  i -->> Caller : Terminate operation
  end

woojiahao commented 3 months ago

@luminousleek Thank you for the review :) I have updated my commit message and addressed the comments!

Re refactoring the issues service: it does appear that a final step for several other methods is to perform an effective DTO mapping from GithubIssue to Issue. Perhaps the team might want to consider a proper logical split between the logic that retrieves data from the Github API and the logic that performs the DTO mapping. I feel that this can help to make the code a lot clearer. It also helps to create more atomic operations for issue querying with the Github API. Just my 2 cents!

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 54.50%. Comparing base (5e7ed48) to head (87d6ae5).

Files	Patch %	Lines
src/app/core/services/issue.service.ts	0.00%	14 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1264 +/- ## ========================================== - Coverage 54.82% 54.50% -0.33% ========================================== Files 105 105 Lines 2858 2873 +15 Branches 503 506 +3 ========================================== - Hits 1567 1566 -1 - Misses 1030 1045 +15 - Partials 261 262 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

CATcher-org / CATcher