-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add project select to the contributors page gf-504 #516
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 project select to the contributors page gf-504 #516
Conversation
…not only selected gf-504
| }); | ||
|
|
||
| useEffect(() => { | ||
| const loadContributors = useCallback(() => { |
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.
a10
apps/backend/src/modules/contributors/contributor.controller.ts
Outdated
Show resolved
Hide resolved
apps/backend/src/modules/contributors/contributor.repository.ts
Outdated
Show resolved
Hide resolved
| } from "../../../../libs/types/types.js"; | ||
| import { type ContributorOrderBy } from "../enums/contributor-order-by.enum.js"; | ||
|
|
||
| type ContributorGetAllRequestDto = { |
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.
Aren't these query parameters? Request dto is for request body
type ContributorGetAllQueryParameters =
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.
You right
apps/backend/src/modules/contributors/contributor.controller.ts
Outdated
Show resolved
Hide resolved
| switch (orderBy) { | ||
| case ContributorOrderBy.CREATED_AT: { | ||
| query.orderBy(ContributorOrderBy.CREATED_AT, SortType.DESCENDING); | ||
| break; | ||
| } | ||
|
|
||
| public async findAllWithoutPagination({ | ||
| contributorName, | ||
| hasHidden = true, | ||
| }: { | ||
| contributorName?: string; | ||
| hasHidden?: boolean; | ||
| }): Promise<{ items: ContributorEntity[] }> { | ||
| const query = this.contributorModel | ||
| .query() | ||
| .select("contributors.*") | ||
| .select( | ||
| raw( | ||
| "COALESCE(ARRAY_AGG(DISTINCT jsonb_build_object('id', projects.id, 'name', projects.name)) FILTER (WHERE projects.id IS NOT NULL), '{}') AS projects", | ||
| ), | ||
| ) | ||
| .leftJoin("git_emails", "contributors.id", "git_emails.contributor_id") | ||
| .leftJoin("activity_logs", "git_emails.id", "activity_logs.git_email_id") | ||
| .leftJoin("projects", "activity_logs.project_id", "projects.id") | ||
| .groupBy("contributors.id") | ||
| .withGraphFetched("gitEmails"); | ||
|
|
||
| if (!hasHidden) { | ||
| query.whereNull("contributors.hiddenAt"); | ||
| case ContributorOrderBy.LAST_ACTIVITY_DATE: { | ||
| query.orderBy( | ||
| ContributorOrderBy.LAST_ACTIVITY_DATE, | ||
| SortType.DESCENDING, | ||
| ); | ||
| break; | ||
| } |
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 don't need all this logic and we can do it this way:
query.orderBy(orderBy, SortType.DESCENDING);
in case we need another sortType, this should be passed to query parameters. And for query params validation itself we need to have a validation schema which will restrict orderBy only to two options
| @@ -0,0 +1,6 @@ | |||
| const ContributorOrderBy = { | |||
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.
| const ContributorOrderBy = { | |
| const ContributorOrderByKey = { |
...ed/src/modules/contributors/libs/validation-schemas/contributor-get-all.validation-schema.ts
Show resolved
Hide resolved
|
I'm sure I miss some buisness logic bugs while resolving conflicts, so I'll try to find them out tomorrow |
what1s1ove
left a comment
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
| return results.map(({ projects, ...projectGroup }) => { | ||
| const [project] = projects; | ||
| return results | ||
| .filter(({ projects }) => projects.length) |
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.
This check for cases when user deletes project, which has some project groups. After such a deletion in the DB project groups are remained in project_groups table, but they relation with projects are lost (corresponding record in projects_to_project_groups are deleted). So without this .filter, project in const [project] = projects; expression is undefined which leads to Reading property 'id' of undefined error.
It would be better to remove this filter after we fix mentioned issue.
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.
we need to remove this. This shouldn't be the case for project groups. We need to create a migration to make the project field required and delete groups by cascade
| CREATED_AT: "created_at", | ||
| LAST_ACTIVITY_DATE: "last_activity_date", |
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 as we agreed, maybe we should use camelCase here? (I think that this might be fixed in ticket regarding quality criteria, but if you'll fix anything else, you might to fix this also).
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.
@Vitaliistf can we fix this in your PR please? we need this pr to be merged to fix deployment
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.
Sure
Features
Added project select above contributors table
Screenshots