-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: add contributors page gf-250 #334
Conversation
…contributors-page
@@ -75,6 +75,22 @@ class ProjectRepository implements Repository { | |||
return item ? ProjectEntity.initialize(item) : null; | |||
} | |||
|
|||
public async findProjectsByContributorId( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public async findProjectsByContributorId( | |
public async findByContributorId( |
contributors.items.map(async (item) => { | ||
const contributor = item.toObject(); | ||
|
||
const projects = await this.projectService.findProjectsByContributorId( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can join projects inside contributors repository findAll method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I thought that such logic should be through services. You mean that I should remove it completely from projectRepository, and create such method in contributorRepository, or make contributorRepository dependent of projectRepository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I was thinking about calculating it as a custom field somehow inside the query in contributors repository, but looks like it will be complicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the problem is that it's not really good to make a separate query for each item. Also, we need to use for of, instead of Promise all, because this may make too many simultaneous db requests, which is bad. And the concern here is that with for of it will work slow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and if there is a possibility to compute or somehow join this inside the contributors query, this would be better, otherwise, we need to use for of here, which can be slow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, alright, I'll try to figure out what we could do with it tomorrow. Resolving a lot of conflicts today got me feeling fried😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm after Lisa's comments
this.contributorRepository = contributorRepository; | ||
this.projectService = projectService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we no longer need this
this.projectService = projectService; |
.select( | ||
raw( | ||
"COALESCE(ARRAY_AGG(DISTINCT jsonb_build_object('id', git_emails.id, 'email', git_emails.email)) FILTER (WHERE git_emails.id IS NOT NULL), '{}') AS git_emails", | ||
), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can join git emails the usual way, with relation mapping, because we will probably need this in another place
.execute(); | ||
|
||
return ContributorEntity.initialize(contributor); | ||
return ContributorEntity.initialize({ ...contributor, projects: [] }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we leave return ContributorEntity.initialize(contributor);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, now that we have one query for that, we can, I forgot to change it. Thanks!
(Branched from #181)