Refactor GithubUser model to no longer encapsulate assignee role #379
Conversation
kokerinks
changed the title
kokerinks
changed the title
There was a problem hiding this comment.
Hi @kokerinks, l left some comments, pls lmk if you have any questions.
| getDataForGroup(issues: Issue[], key: GithubUser): Issue[] { | ||
| if (key === GithubUser.NO_ASSIGNEE) { | ||
| return this.getUnassignedData(issues); | ||
| } | ||
|
|
||
| return this.getDataAssignedToUser(issues, key); | ||
| getDataForGroup(issues: Issue[], key: Assignee): Issue[] { | ||
| return issues.filter( | ||
| (issue) => issue.assignees.includes(key.login) || (key === Assignee.WithoutAssignee && issue.assignees.length === 0) | ||
| ); | ||
| } |
There was a problem hiding this comment.
In WATcher, the issue model can represent either a GitHub Issue or a GitHub Pull Request. Therefore, in getDataAssignedToUser, we use different filtering logic: for GitHub Issues, we retrieve those assigned to the target; for GitHub Pull Requests, we retrieve those created by the target.
| import { Group } from './github/group.interface'; | ||
|
|
||
| /** | ||
| * Represents an assignee and its attributes fetched from Github. | ||
| */ | ||
| export class Assignee implements Group { | ||
| static WithoutAssignee: Assignee = new Assignee({ login: 'Issues/PRs without an assignee', avatar_url: null }); | ||
| login: string; | ||
| avatarUrl: string; | ||
|
|
||
| constructor(assignee: { login: string; avatar_url: string }) { | ||
| this.login = assignee.login; | ||
| this.avatarUrl = assignee.avatar_url; | ||
| } | ||
|
|
||
| public equals(other: any) { | ||
| if (!(other instanceof Assignee)) { | ||
| return false; | ||
| } | ||
| return this.login === other.login; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't see the need for a new class to represent an assignee since an assignee can only be a GitHub User. Unless the assignee has different behaviors or fields from a GitHub user, creating a new class seems unnecessary. Additionally, you created a new service for the assignee, which just parses data from the GitHub user.
If the concern is about methods that only require the "login" and "avatarUrl" fields shouldn't have access to other fields of Github User, a simpler solution would be to create an interface called Assignee with these fields and have the GitHub user implement this interface. This way, methods requiring an assignee can use the Assignee type, ensuring only the assignee-specific fields are accessed.
I might be wrong about this, so please feel free to discuss further if you have different considerations.
There was a problem hiding this comment.
@NereusWB922 okay, after working deeper on the filter PR I think i have a better understanding of the role of each of the models and services.. will try to incorporate the new filter without using a new assignee model
that said, having a new service for assignee is fine right? I'll just rewrite it so that it uses githubUsers instead of the new assignee model class
There was a problem hiding this comment.
Hi @kokerinks, then in this case, you can just use the method getUsersAssignable from github service right? New service seems unnecessary also.
There was a problem hiding this comment.
hmm i think it would make the code less easy to understand though. i intentionally designed assignee.service.ts to be similar in structure to milestone.service.ts, since they both have a similar purpose of handling logic for their respective filters.assignee.service.ts currently also "caches" the assignees taken from getUsersAssignable, which will help to reduce calls to github APIs.
There are some options i see here:
- Use
getUsersAssignableto retrieve each time and without any caching
- will cause structure of
assigneefilter implementation to differ from milestone - will result in more github api calls
- Implement caching in
github.service.ts
- code structure would not be consistent with
milestone.service.ts, where the milestones are "cached" at
- Implement caching in a new
assignee.service.ts
- will result in a new file, but will align better with the current milestone filter implementation in terms of code structure
Personally, I think having the new file is necessary.. i understand the potential concern of having too many service files, but maybe we can reorganise the services into new folders to declutter the services folder.
bit of a read, let me know what you think!
There was a problem hiding this comment.
Hi @kokerinks, thanks for the detailed explanation. I can see where you are coming from and agree that having the logic related to filtering in the GitHub service would make it too complicated. Please go ahead with your idea but just remove the Assignee model. Thanks.
There was a problem hiding this comment.
Just let me know when this pr is ready for review again. Thanks
There was a problem hiding this comment.
Okay, will be closing this PR and reimplementing it in #381 alongside the assignee filter implementation. will tag you in it for review once its ready
|
Reimplementing it in #381 alongside the assignee filter implementation |
Summary:
This PR is in preparation for the enhancement proposed in the issue #342
Before this PR, the
GithubUsermodel was responsible for storing information for both the authentication as well as details of assignees in each issue/PR. This PR turns them into two separate classes, keepingGitHubUserfor authentication and moving the details of assignees to a new classAssignee.This is done to ensure separation of roles between models. This will also improve code quality for future PRs related to the role of
Assignees.Type of change:
Changes Made:
AssigneeMilestonemodelGithubUseris used as an assignee withAssigneeassignee-grouping-strategy, where GithubUser is used heavily as an assigneeProposed Commit Message:
Checklist: