Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect Github REST API response types #1321

Conversation

IzN432
Copy link
Contributor

@IzN432 IzN432 commented Feb 10, 2025

Summary:

Fixes #1314

Changes Made:

  • Added GithubRestIssue, a type to accept issue responses from the REST API
  • Replaced the type GithubIssue with GithubRestIssue where relevant
  • Added a factory method in GithubIssue to convert GithubRestIssue to GithubIssue

Proposed Commit Message:

Add type to correctly process issues from the REST API

Issue responses from the REST API are being accepted as the GithubIssue
class. This is incorrectly typed since the REST API returns a lower
cased state, while GithubIssue expects an upper cased state.

This causes an inconsistency when updating the issues in
issue.service.ts with the response from these REST API queries, which
is done during close and reopen. This prevents us from filtering based
on the state correctly.

Let's add a new type called GithubRestIssue that accurately represents
the issue response from the REST API. We then add a factory method in
GithubIssue that creates a GithubIssue from a GithubRestIssue and
replace all incorrect GithubIssue usages with GithubRestIssue.

body: string;
created_at: string;
labels: Array<GithubLabel>;
state: 'open' | 'closed';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some code tracing, there is a RestGithubIssueState type in github-issue-filter.model.ts:
Screenshot 2025-02-10 at 1 50 07 PM

I think it would be better to reuse this type instead of hard coding the state values here

Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work on the factory function. Now restoring and deleting issues is nearly instant!

I noticed you missed some replacing some GithubIssue in some areas but it does not change any functionalities as neither the state field or the issue was accessed by the rest of the program.

Thus, I will approve this but I'll let you decide if you wish to change them.

  1. toFetchIssue
  2. getIssuesAPICall (this is binded w toFetchIssues)

@IzN432
Copy link
Contributor Author

IzN432 commented Feb 10, 2025

Updated toFetchIssues and getIssuesAPICall as well as toFetchIssue and the IssueCacheManager methods

Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Eclipse-Dominator Eclipse-Dominator merged commit 6e3b82a into CATcher-org:feature-bug-trimming Feb 10, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants