feat: add an ability to mark contributors as hidden gf-346 #400
Conversation
| id="isHidden" | ||
| name="isHidden" | ||
| /> | ||
| <span>Is hidden</span> |
There was a problem hiding this comment.
I understand that you didn't choose this name, but am I the only one who thinks "Is Hidden" sounds more like developer language than something suited for a user interface?
"Do Not Track", or perhaps just "Hidden"
There was a problem hiding this comment.
good suggestion, I like do not track option (on the backend side let's keep it as it is now)
kolibri753
requested review from
what1s1ove,
liza-veis and
yevhenii-hladkov
as code owners
| contributorId: contributorId ?? "", | ||
| contributorIsHidden: contributor ? contributor.isHidden : false, | ||
| contributorName: contributorName ?? "", |
There was a problem hiding this comment.
maybe we can return contributor as an object and id, isHidden, and name as properties, or is there an issue? cc @sandrvvu
| const visibleActivityLogs = activityLogs.filter( | ||
| (log) => !log.contributorIsHidden, | ||
| ); |
There was a problem hiding this comment.
this should be filtered inside backend query
There was a problem hiding this comment.
Is it mandatory to modify query like this builder.select("id", "name").where("isHidden", false); or can it be just check in service if (!contributor.isHidden), not sure it is needed to add isHidden to contributor on GitEmailModel.
(I'll push it with just a check for a better look)
There was a problem hiding this comment.
we don't need isHidden in GitEmailModel for now, it's better to use query builder
| id="isHidden" | ||
| name="isHidden" | ||
| /> | ||
| <span>Is hidden</span> |
There was a problem hiding this comment.
good suggestion, I like do not track option (on the backend side let's keep it as it is now)
|
|
||
| const IsHiddenHeader = (): JSX.Element => ( | ||
| <div className={styles["header-with-icon"]}> | ||
| Is Hidden <Icon height={15} name="circleQuestion" width={15} /> |
There was a problem hiding this comment.
In the design, this icon is meant to be with a tooltip, but if we rename the column on UI to Do Not Track, then I think we can remove this icon at all, especially taking into consideration that it's described inside edit modal
...c/pages/contributors/libs/helpers/get-contributor-columns/get-contributor-columns.helper.tsx
Show resolved
Hide resolved
| @@ -166,6 +166,10 @@ const Project = (): JSX.Element => { | |||
| userPermissions, | |||
| ); | |||
|
|
|||
| const visibleProjectContributors = projectContributors.filter( | |||
There was a problem hiding this comment.
same, should be returned this way from the backend
| return { | ||
| commitsNumber: commitsArray, | ||
| contributorId: contributorId ?? "", | ||
| contributorIsHidden: contributor ? contributor.isHidden : false, |
There was a problem hiding this comment.
| contributorIsHidden: contributor ? contributor.isHidden : false, | |
| contributorIsHidden: contributor?.isHidden ?? false, |
| @@ -1,5 +1,6 @@ | |||
| type ActivityLogQueryParameters = { | |||
| endDate: string; | |||
| hasHidden?: boolean; | |||
There was a problem hiding this comment.
I meant to only pass this parameter as false on activity log service level, similarly, we did for hasDeleted parameter, where we needed to find with deleted users in one place and without them in another place. And don't we need to pass this parameter to findAllByProjectId?
| const TABLE_NAME = "contributors"; | ||
|
|
||
| const ColumnName = { | ||
| IS_HIDDEN: "is_hidden", |
There was a problem hiding this comment.
let's rename this column to hidden_at and record datetime
There was a problem hiding this comment.
so I'm changing it everywhere, right?
| void handleSubmit((formData: { isHidden: boolean; name: string }) => { | ||
| const hiddenAtValue = isHiddenValue ? new Date().toISOString() : null; |
There was a problem hiding this comment.
Why don't you use isHidden instead of isHiddenValue and useFormWatch?
There was a problem hiding this comment.
isHidden (formData.isHidden) resulted in undefined, will investigate it more
There was a problem hiding this comment.
Maybe we need to pass a default value? there is no need for setValue here, usual checkbox should be handled as inputs using useForm default flow
| .modifyGraph("gitEmail.contributor", (builder) => { | ||
| builder.select("id", "name"); | ||
| builder.select("id", "name", "HiddenAt"); |
There was a problem hiding this comment.
| builder.select("id", "name", "HiddenAt"); | |
| builder.select("id", "name", "hiddenAt"); |
| public async findAllWithoutPagination( | ||
| hasHidden?: boolean, | ||
| ): Promise<ContributorGetAllResponseDto> { |
There was a problem hiding this comment.
It's may be not clear what boolean parameter is used for outside of this service
| public async findAllWithoutPagination( | |
| hasHidden?: boolean, | |
| ): Promise<ContributorGetAllResponseDto> { | |
| public async findAllWithoutPagination({ | |
| hasHidden, | |
| }: { hasHidden?: boolean }): Promise<ContributorGetAllResponseDto> { |
For example, you call this.contributorService.findAllWithoutPagination(false)); -- when we look at this call it is hard to guess what false may be used for.
Same for findAllByProjectId
| const handleFormSubmit = useCallback( | ||
| (event_: React.BaseSyntheticEvent): void => { | ||
| void handleSubmit((formData: { name: string }) => { | ||
| const payload: ContributorPatchRequestDto = { name: formData.name }; | ||
| void handleSubmit((formData: { isHidden: boolean; name: string }) => { |
There was a problem hiding this comment.
Why don't we just destucture it
| void handleSubmit((formData: { isHidden: boolean; name: string }) => { | |
| void handleSubmit(({ isHidden: boolean; name: string }) => { |
BTW, no matter you annotate formData with { isHidden: boolean; name: string } type, it still contains gitEmails, name and projects
There was a problem hiding this comment.
yep, but if I'm not mistaken void handleSubmit(({ isHidden, name }) => {
There was a problem hiding this comment.
agree with this option, void handleSubmit(({ isHidden, name }) => {, from the code it looks like formData should already have an appropriate type, so we don't need to define it again
| cell: ({ row: { original: contributor } }) => | ||
| contributor.hiddenAt ? ( | ||
| <Icon height={18} name="check" width={18} /> | ||
| ) : null, |
There was a problem hiding this comment.
To render nothing we are using this approach
| ) : null, | |
| ) : <></>, |
| @@ -8,6 +8,7 @@ import { type ContributorPatchRequestDto } from "../types/types.js"; | |||
|
|
|||
| const contributorPatch: z.ZodType<ContributorPatchRequestDto> = z | |||
| .object({ | |||
| hiddenAt: z.union([z.string().nullable(), z.null()]), | |||
There was a problem hiding this comment.
Can't we do it like this?
| hiddenAt: z.union([z.string().nullable(), z.null()]), | |
| hiddenAt: z.string().nullable(), |
There was a problem hiding this comment.
or at least
| hiddenAt: z.union([z.string().nullable(), z.null()]), | |
| hiddenAt: z.union([z.string(), z.null()]), |
|
Don't forget to update |
| @@ -86,9 +93,10 @@ class ContributorRepository implements Repository { | |||
| } | |||
|
|
|||
| public async findAllByProjectId( | |||
| hasHidden: boolean = true, | |||
There was a problem hiding this comment.
let's pass it as a second parameter, it would be more logical to me
| void handleSubmit((formData: { isHidden: boolean; name: string }) => { | ||
| const hiddenAtValue = isHiddenValue ? new Date().toISOString() : null; |
There was a problem hiding this comment.
Maybe we need to pass a default value? there is no need for setValue here, usual checkbox should be handled as inputs using useForm default flow
| @@ -1,4 +1,5 @@ | |||
| type ContributorPatchRequestDto = { | |||
| hiddenAt: null | string; | |||
There was a problem hiding this comment.
for request it's better to pass isHidden, and set the timestamp on the backend side
| hiddenAt: null | string; | |
| isHidden: boolean; |
| @@ -1,5 +1,6 @@ | |||
| type ContributorPatchResponseDto = { | |||
| id: number; | |||
| isHidden?: boolean; | |||
There was a problem hiding this comment.
Don't we return hiddenAt in response?
Changes:
Screenshots: