feat(condo): DOMA-10516 reassign employee ticket #5563

tolmachev21 · 2024-12-02T16:06:37Z

apps/condo/domains/common/constants/featureflags.js

toplenboren · 2024-12-03T07:50:09Z

apps/condo/domains/ticket/queries/Ticket.graphql

@@ -259,8 +259,31 @@ query getContactTickets ($contactId: ID!) {
    }
 }

+query getOrganizationEmployeeTickets($where: TicketWhereInput!, $first: Int, $skip: Int) {


I'd have named it getTicketIdsByEmployeeId, seems more true to its nature

toplenboren · 2024-12-03T07:54:41Z

apps/condo/domains/ticket/schema/Ticket.test.js

@@ -12,6 +12,7 @@ const {
    expectToThrowAuthenticationErrorToObjects,


You should add more tests, right now added tests do not cover added bulk operations

Really you should just take all tests for CRUD operations and duplicate it for bulk cases.

For example:

I as employee try to update tickets from my org

I as employee try to update tickets from my org, but I do not have the role (i'm not Administrator and do not have canManageTickets permission)

I as employee try to update tickets from other org ...

I as support try to update multiple tickets
And so on

Do not forget about complex cases, like this:

I as hacker try to update multiple tickets: 2 tickets are from my organization, and 1 ticket is from other organization

I try to update stragne random fields

toplenboren · 2024-12-03T07:57:12Z

apps/condo/lang/en/en.json

@@ -702,7 +702,7 @@
  "employee.AllSpecializations": "all specializations",
  "employee.BlockUser": "Block user",
  "employee.CanNotBlockYourself": "(you can't block yourself)",
-  "employee.ConfirmDeleteMessage": "All data about him will be erased forever",
+  "employee.ConfirmDeleteMessage": "The data will not be recoverable and the employee will not be able to log in to the Doma.ai platform.",


No, you should not mention Doma.ai in open source translations.

toplenboren · 2024-12-03T07:57:27Z

apps/condo/lang/en/en.json

@@ -719,6 +719,15 @@
  "employee.emptyList.description": "Your employer has blocked you from the organization",
  "employee.emptyList.title": "You are blocked",
  "employee.isBlocked": "blocked",
+  "employee.reassignTickets.title": "Who should I assign employee requests to?",


toplenboren · 2024-12-03T08:01:37Z

apps/condo/lang/ru/ru.json

@@ -702,7 +702,7 @@
  "employee.AllSpecializations": "Все типы работ",
  "employee.BlockUser": "Заблокировать пользователя",
  "employee.CanNotBlockYourself": "(вы не можете заблокировать себя)",
-  "employee.ConfirmDeleteMessage": "Все данные о нём сотрутся навсегда",
+  "employee.ConfirmDeleteMessage": "Данные нельзя будет восстановить, а сотрудник не сможет войти на платформу Doma.ai",


toplenboren · 2024-12-03T08:02:49Z

apps/condo/pages/employee/[id]/index.tsx

            updateEmployeeAction={updateEmployeeAction}
+            activeTicketsOrganizationEmployeeCount={get(activeTicketsOrganizationEmployeeCount, 'meta.count', 0)}


why 0 is default here? Default means something went wrong with the query, is it not?

Does this mean that there should be no default value or should there be another value? Or something else?

toplenboren · 2024-12-03T08:06:37Z

apps/condo/domains/ticket/access/Ticket.js

-            if (!itemId) return false
-            const ticket = await getById('Ticket', itemId)
-            organizationId = get(ticket, 'organization', null)
+            if (isBulkRequest) {


are there any other checks for singular update case?

These cases are below. There are cases when updating and creating one ticket - the property must belong to the same organization for which the ticket is created

toplenboren · 2024-12-03T08:06:54Z

apps/condo/domains/organization/components/DeleteButtonWithReassigmentEmployeeModal.tsx

+                    message: notificationTitle('Ошибка'),
+                    description: notificationMessage('Повторите попытку'),


toplenboren · 2024-12-03T08:07:14Z

apps/condo/domains/organization/components/DeleteButtonWithReassigmentEmployeeModal.tsx

+                    console.log(e)
+                    console.error(e.friendlyDescription)


nomerdvadcatpyat · 2024-12-03T07:14:05Z

apps/condo/domains/organization/components/DeleteButtonWithReassigmentEmployeeModal.tsx

+
+    const [notificationApi, contextHolder] = notification.useNotification()
+
+    const filterTicket = `{"search":"${employeeName}", "status":["new_or_reopened","processing","completed","deferred"]}`


The search filter includes a search across multiple fields (e.g clientName, details). If they contain the substring ${employeeName}, then the ticket will also be displayed.
Mb add new custom filter in useTicketTableFilters which filters by the id of the executor or assignee?
Or mb remove this logic?

And you can use enum from '@app/condo/schema' for status types

nomerdvadcatpyat · 2024-12-03T07:22:08Z

apps/condo/domains/ticket/queries/Ticket.graphql

@@ -259,8 +259,31 @@ query getContactTickets ($contactId: ID!) {
    }
 }

+query getOrganizationEmployeeTickets($where: TicketWhereInput!, $first: Int, $skip: Int) {


Make a more specific name for your use, for example getOrganizationEmployeeTicketsForReassign. And specific arguments instead where: ($userId: ID!, !organizationId: ID!, $first ... )

nomerdvadcatpyat · 2024-12-03T07:23:36Z

apps/condo/domains/organization/components/DeleteButtonWithReassigmentEmployeeModal.tsx

+                organization: { id: organizationId },
+                OR: [
+                    { assignee: { id: employeeUserId } },
+                    { executor: { id: employeeUserId } },
+                ],
+                status: {
+                    type_in: [
+                        TicketStatusTypeType.NewOrReopened,
+                        TicketStatusTypeType.Processing,
+                        TicketStatusTypeType.Completed,
+                        TicketStatusTypeType.Deferred,
+                    ],
+                },


you can pass userId and organizationId as query arguments and describe this where in graphql query

nomerdvadcatpyat · 2024-12-03T07:26:01Z

apps/condo/domains/organization/components/DeleteButtonWithReassigmentEmployeeModal.tsx

+        },
+        fetchPolicy: 'no-cache',
+    })
+    const [updateTicketsWithReassignOrganizationEmployee] = useUpdateTicketsMutation()


Suggested change

const [updateTicketsWithReassignOrganizationEmployee] = useUpdateTicketsMutation()

const [updateTickets] = useUpdateTicketsMutation()

nomerdvadcatpyat · 2024-12-03T07:31:03Z

apps/condo/domains/organization/components/DeleteButtonWithReassigmentEmployeeModal.tsx

+                    message: notificationTitle('Ошибка'),
+                    description: notificationMessage('Повторите попытку'),


nomerdvadcatpyat · 2024-12-03T07:53:35Z

apps/condo/domains/ticket/access/Ticket.js

+                id_in: itemIds || [itemId],
+                deletedAt: null,
+            })
+            organizationId = uniq(tickets.map(ticket => get(ticket, 'organization', null)))


Now it is organizationIds array

apps/condo/domains/ticket/queries/Ticket.graphql

nomerdvadcatpyat · 2024-12-03T08:01:12Z

packages/icons/src/components/CallCenter.tsx

It looks like changes that are not related to this pr. This and icons below

apps/condo/domains/ticket/access/Ticket.js

nomerdvadcatpyat · 2024-12-03T08:09:09Z

apps/condo/domains/organization/components/DeleteButtonWithReassigmentEmployeeModal.tsx

+            setIsDeleting(false)
+            notificationApi.success({
+                message: notificationTitle(NotificationSuccessLabel),
+                description: notificationMessage(NotificationTitleLabel, updatedTicketsCount),
+                duration: 0,
+                key: 'reassignTicket',


runMutation has onCompleted and OnCompletedMsg args, look at them

Alllex202

Don't use lodash.get in typescript (except when it's really necessary). It outputs bad types and doesn't show errors during development. You'll only learn about errors in production from real users.

And since we use typescript, please add good types. It's not that hard :)

Alllex202 · 2024-12-04T09:23:12Z

apps/condo/pages/employee/[id]/index.tsx

@@ -278,13 +296,14 @@ export const EmployeePageContent = ({
    )
 }

-export const EmployeeInfoPage: PageComponentType = () => {
+export const EmployeeInfoPage = () => {


bring the types back

Alllex202 · 2024-12-04T09:25:46Z

apps/condo/pages/employee/[id]/index.tsx

    const intl = useIntl()
    const UpdateEmployeeMessage = intl.formatMessage({ id: 'employee.UpdateTitle' })
    const ErrorMessage = intl.formatMessage({ id: 'errors.LoadingError' })

+    const organizationId = get(organization, 'id', null)


You should not use lodash.get, you can write the same thing using optional chains

Alllex202 · 2024-12-04T09:27:23Z

apps/condo/pages/employee/[id]/index.tsx

    const { objs: organizationEmployeeSpecializations } = OrganizationEmployeeSpecialization.useObjects({
        where: {
            employee: { id: employeeId },
        },
    })

+    const employeeUserId = get(employee, 'user.id', null)


don't use get in typescript

Alllex202 · 2024-12-04T09:37:42Z

apps/condo/pages/employee/[id]/index.tsx

+    employeeUserId,
+    organizationId,


What is this for?
There is no need to pass this data from the parent element. The employee is already passed.
The organization can also be get on the spot from an employee or hook (you can only delete an employee from active organization)

Alllex202 · 2024-12-04T09:45:36Z

apps/condo/pages/employee/[id]/index.tsx

+    const { data: activeTicketsOrganizationEmployeeCount } = useGetTicketsCountQuery({
+        variables: {
+            where: {
+                organization: { id: organizationId },
+                OR: [
+                    { assignee: { id: employeeUserId } },
+                    { executor: { id: employeeUserId } },
+                ],
+                status: {
+                    type_in: [
+                        TicketStatusTypeType.NewOrReopened,
+                        TicketStatusTypeType.Processing,
+                        TicketStatusTypeType.Completed,
+                        TicketStatusTypeType.Deferred,
+                    ],
+                },
+            },
+        },
+    })


Better make a separate request for this in .graphql file

like getOrganizationEmployeeTicketsForReassign, but for count

Alllex202 · 2024-12-04T10:39:00Z

apps/condo/domains/ticket/access/Ticket.js

+            }
+
+            const tickets = await find('Ticket', {
+                id_in: itemIds || [itemId],


Suggested change

id_in: itemIds || [itemId],

id_in: isBulkRequest ? itemIds : [itemId],

Alllex202 · 2024-12-04T10:45:04Z

apps/condo/domains/ticket/access/Ticket.js

+            originalInput.forEach((updatedTicket) => {
+                const propertyId = get(updatedTicket, ['property', 'connect', 'id'])
+                if (propertyId) propertyIds.push(itemId)
+            })


why not just use map?

Suggested change

originalInput.forEach((updatedTicket) => {

const propertyId = get(updatedTicket, ['property', 'connect', 'id'])

if (propertyId) propertyIds.push(itemId)

})

propertyIds = originalInput.map(ticket => get(ticket, ['property', 'connect', 'id'], null)).filter(Boolean)

Alllex202 · 2024-12-04T10:50:07Z

apps/condo/domains/ticket/access/Ticket.js

+                const propertyId = get(updatedTicket, ['property', 'connect', 'id'])
+                if (propertyId) propertyIds.push(itemId)
+            })
+            if (!isEmpty(propertyIds) && propertyIds.length !== originalInput.length) return false


Is this definitely the correct condition?
You can pass data without a property

Suggested change

if (!isEmpty(propertyIds) && propertyIds.length !== originalInput.length) return false

if (!isEmpty(propertyIds) return false

Alllex202 · 2024-12-04T10:57:27Z

apps/condo/domains/ticket/access/Ticket.js


-            return organizationId === get(property, 'organization')
+            return isEqual(organizationIds, propertyOrganizationIds)


This condition will not work correctly if the order of the elements is different.
You should fix this

It would be nice to add tests that check different cases. There aren't any now :(

Alllex202 · 2024-12-04T10:58:49Z

apps/condo/domains/ticket/schema/Ticket.test.js

+const TICKET_OPEN_STATUS_ID = '6ef3abc4-022f-481b-90fb-8430345ebfc2'
+const TICKET_OTHER_SOURCE_ID = '7da1e3be-06ba-4c9e-bba6-f97f278ac6e4'


we have STATUS_IDS const, use it

tolmachev21 · 2024-12-11T17:16:40Z

apps/condo/domains/ticket/access/Ticket.js

@@ -111,6 +115,30 @@ async function canManageTickets (args) {
            return ticket.client === user.id
        }
    } else {
+        if (isBulkRequest && operation === 'create') return false
+


I'll add a TODO here with task DOMA-10832

tolmachev21 · 2024-12-11T17:18:21Z

apps/condo/domains/ticket/queries/Ticket.graphql

@@ -91,6 +91,24 @@ query getTicketsCount ($where: TicketWhereInput!) {
    }
 }

+query getOrganizationEmployeeTicketsCountForReassign ($userId: ID!, $organizationId: ID!) {


Do I need to change to?

Suggested change

query getOrganizationEmployeeTicketsCountForReassign ($userId: ID!, $organizationId: ID!) {

query getOrganizationEmployeeTicketsCountForReassignment ($userId: ID!, $organizationId: ID!) {

tolmachev21 · 2024-12-11T17:20:24Z

apps/condo/domains/ticket/schema/Ticket.test.js

@@ -1448,6 +1464,528 @@ describe('Ticket', () => {
        })
    })

+    describe('Bulk-operation', () => {


It's normal that I put the logic in a separate describe?

tolmachev21

I'll also add a commit with the changes to callcenter here

nomerdvadcatpyat · 2024-12-12T07:18:28Z

apps/condo/domains/ticket/access/Ticket.js

@@ -111,6 +115,30 @@ async function canManageTickets (args) {
            return ticket.client === user.id
        }
    } else {
+        if (isBulkRequest && operation === 'create') return false


you already check it in line 73

Yes, that's right. I thought about leaving the check for better understanding and easier scaling of the code in the future. But if it's unnecessary, I'll remove it. Let me know

I think it's better to remove it and leave in one place

nomerdvadcatpyat · 2024-12-12T07:20:57Z

apps/condo/domains/ticket/access/Ticket.js

+            if (!originalInput.every((updateItem) => {
+                for (const key in updateItem.data) {
+                    if (!BULK_UPDATE_ALLOWED_FIELDS.includes(key)) return false
+                }
+                return true
+            })) return false


do we really need this?

Now, with a single update, we have a check of the address for its compliance with the organization. With a bulk update, we must also support this logic. However, the task of updating and maintaining all possible scenarios of bulk operations is a different task. Therefore, we decided to temporarily limit the update to only two fields - executor and assignee

nomerdvadcatpyat · 2024-12-12T07:34:40Z

apps/condo/pages/employee/[id]/index.tsx

+    useEffect(() => {
+        if (isFirstRender.current) {
+            isFirstRender.current = false
+            return
+        }
+        if (!loading && !employee) {
+            push('/employee/')
+        }
+    }, [employee, loading, push])


What is it?

It is necessary that we do not have pages like these

This employee was deleted

This is something strange, better tell the user that the employee was not found.
This will be much clearer than a redirect

I haven't seen similar logic with redirect

tolmachev21 · 2024-12-12T08:40:38Z

apps/condo/domains/ticket/access/Ticket.js

+                }
+                return true
+            })) return false
+


We may have crooked bulk updates of employees. This is true. I want to roll out this feature and then take on the task of updating accesses. However, if this is really bad, then I can make a crutch. We will check that only one user is updated. And only he will be in the check for compliance with the employee's organization - the ticket organization

Let me know what is the best way to proceed

toplenboren · 2024-12-06T10:57:23Z

apps/condo/domains/organization/components/DeleteButtonWithReassigmentEmployeeModal.tsx

+        return resultObj
+    }
+
+    const notificationTitle = useCallback((notificationStatus: React.ReactNode) => <Typography.Text strong>{notificationStatus}</Typography.Text>, [])


if this is a function, its name should start from verb, like getNotificationTitle

If this is a component – just make a component, maybe in this file

toplenboren · 2024-12-06T10:59:56Z

apps/condo/domains/ticket/queries/Ticket.graphql

@@ -91,6 +91,24 @@ query getTicketsCount ($where: TicketWhereInput!) {
    }
 }

+query getOrganizationEmployeeTicketsCountForReassign ($userId: ID!, $organizationId: ID!) {


Suggested change

query getOrganizationEmployeeTicketsCountForReassign ($userId: ID!, $organizationId: ID!) {

query getOrganizationEmployeeTicketsCountByUserAndOrgId ($userId: ID!, $organizationId: ID!) {

but this name is ok for me

toplenboren · 2024-12-12T15:57:44Z

apps/condo/domains/organization/components/DeleteEmployeeButtonWithReassignmentModel.tsx

+    const notificationTitle = useCallback((notificationStatus: React.ReactNode) => <Typography.Text strong>{notificationStatus}</Typography.Text>, [])
+    const notificationMessage = useCallback((notificationTitleLabel: React.ReactNode, updatedTicketsCount = null) => {


If it is a function (which useCallback is) then it should be named as a function – getNotificationMessage / getNotificationTitle

Otherwise you should use useMemo

toplenboren · 2024-12-12T15:58:59Z

apps/condo/domains/organization/components/DeleteEmployeeButtonWithReassignmentModel.tsx

+        runMutation(
+            {
+                action: softDeleteAction,
+                onError: (e) => { throw e },


So, if error happens, whole page will fail?

No, then it will work OnErrorMsg

toplenboren · 2024-12-12T16:00:39Z

apps/condo/domains/organization/components/DeleteEmployeeButtonWithReassignmentModel.tsx

+                })
+
+                updatedTicketsCount += reassignedTickets?.tickets?.length
+                await sleep(1000)


Please move this to const so this is more readable

toplenboren · 2024-12-12T16:03:37Z

apps/condo/domains/ticket/constants.js

@@ -55,6 +55,25 @@ const MAX_COMMENT_LENGTH = 700

 const DEFAULT_DEFERRED_DAYS = 30

+const TICKET_OTHER_SOURCE_ID = '7da1e3be-06ba-4c9e-bba6-f97f278ac6e4'


Why do we need this const?

In tests, I need to check operations for bulk ticket creation. There is no function for creating bulk tickets in test utilities. And I did not write it, because we decided not to support bulk ticket creation. And I need this constant only for creating a ticket that cannot be created

toplenboren · 2024-12-12T16:04:38Z

apps/condo/domains/ticket/schema/Ticket.test.js

+        })
+
+        // Should an admin have access to bulk create tickets?
+        test.skip('admin: cannot bulk create Tickets', async () => {


toplenboren · 2024-12-12T16:06:35Z

apps/condo/lang/en/en.json

+  "employee.reassignTickets.title": "Who should I assign employee requests to tickets?",
+  "employee.reassignTickets.buttonContent.deleteUser": "Remove employee without reassignment",
+  "employee.reassignTickets.buttonContent.reassignTickets": "Delete employee and reassign requests",
+  "employee.reassignTickets.alert.title": "{employeeName} is assigned to multiple tickets ({activeTicketsOrganizationEmployeeCount}",


toplenboren · 2024-12-12T16:07:02Z

apps/condo/lang/ru/ru.json

+  "employee.reassignTickets.title": "На кого назначить заявки сотрудника?",
+  "employee.reassignTickets.buttonContent.deleteUser": "Удалить сотрудника без переназначения",
+  "employee.reassignTickets.buttonContent.reassignTickets": "Удалить сотрудника и переназначить заявки",
+  "employee.reassignTickets.alert.title": "{employeeName} назначен на несколько заявок ({activeTicketsOrganizationEmployeeCount}",


toplenboren · 2024-12-12T16:09:22Z

apps/condo/pages/employee/[id]/index.tsx

    const { objs: organizationEmployeeSpecializations } = OrganizationEmployeeSpecialization.useObjects({
        where: {
            employee: { id: employeeId },
        },
    })

+    const { data: activeTicketsOrganizationEmployeeCount } = useGetOrganizationEmployeeTicketsCountForReassignQuery({
+        variables: {
+            userId: employee?.user?.id,


this means that if userId is undefined for some reason, we will fire a gql request and will get 400 bad request error

Alllex202 · 2024-12-12T10:35:53Z

apps/condo/domains/common/constants/featureflags.js

@@ -28,6 +28,7 @@ const NEWS_SHARING_TEMPLATES = 'news-sharing-templates'
 const SERVICE_PROBLEMS_ALERT = 'service-problems-alert'
 const TICKET_AUTO_ASSIGNMENT_MANAGEMENT = 'ticket-auto-assignment-management'
 const POLL_TICKET_COMMENTS = 'poll-ticket-comments'
+const REASSIGN_EMPLOYEE_TICKETS = 'reassign-employee-tickets'


just a question, why do we want to use feature flags for this feature?

If they start dudosing us with this feature, we can turn it off

Alllex202 · 2024-12-13T08:47:31Z

apps/condo/domains/organization/components/DeleteEmployeeButtonWithReassignmentModel.tsx

+    buttonContent: string
+    softDeleteAction: IUseSoftDeleteActionType<OrganizationEmployee>
+    disabled?: boolean
+    employee: OrganizationEmployee


Suggested change

employee: OrganizationEmployee

employee: Pick<OrganizationEmployee, 'name'> & { organization?: Pick<OrganizationEmployee['organization'], 'id'>, user?: Pick<OrganizationEmployee['organization'], 'id'> }

When using new hooks this type will be automatically generated

Alllex202 · 2024-12-13T08:48:03Z

apps/condo/domains/organization/components/DeleteEmployeeButtonWithReassignmentModel.tsx

+import { searchEmployeeUser } from '@condo/domains/ticket/utils/clientSchema/search'
+
+
+interface IDeleteEmployeeButtonWithReassignmentModel {


Suggested change

interface IDeleteEmployeeButtonWithReassignmentModel {

type DeleteEmployeeButtonWithReassignmentModelProps = {

Alllex202 · 2024-12-13T09:00:46Z

apps/condo/domains/organization/components/DeleteEmployeeButtonWithReassignmentModel.tsx

+
+    const [notificationApi, contextHolder] = notification.useNotification()
+
+    const employeeUserId = employee?.user?.id


Suggested change

const employeeUserId = employee?.user?.id

const employeeUserId = employee?.user?.id || null

Alllex202 · 2024-12-13T09:00:58Z

apps/condo/domains/organization/components/DeleteEmployeeButtonWithReassignmentModel.tsx

+    const [notificationApi, contextHolder] = notification.useNotification()
+
+    const employeeUserId = employee?.user?.id
+    const employeeOrganizationId = employee?.organization?.id


Suggested change

const employeeOrganizationId = employee?.organization?.id

const employeeOrganizationId = employee?.organization?.id || null

Alllex202 · 2024-12-13T09:32:06Z

apps/condo/pages/employee/[id]/index.tsx

+    const { data: activeTicketsOrganizationEmployeeCount } = useGetOrganizationEmployeeTicketsCountForReassignQuery({
+        variables: {
+            userId: employee?.user?.id,
+            organizationId: employee?.organization?.id,


Suggested change

organizationId: employee?.organization?.id,

organizationId: employee?.organization?.id || null,

Alllex202 · 2024-12-13T09:32:47Z

apps/condo/pages/employee/[id]/index.tsx

+    const { data: activeTicketsOrganizationEmployeeCount } = useGetOrganizationEmployeeTicketsCountForReassignQuery({
+        variables: {
+            userId: employee?.user?.id,


add skip if userId or organizationId is empty

Alllex202 · 2024-12-13T09:34:30Z

apps/condo/pages/employee/[id]/index.tsx

    const { objs: organizationEmployeeSpecializations } = OrganizationEmployeeSpecialization.useObjects({
        where: {
            employee: { id: employeeId },
        },
    })

+    const { data: activeTicketsOrganizationEmployeeCount } = useGetOrganizationEmployeeTicketsCountForReassignQuery({


How about waiting for the data to load?

Alllex202 · 2024-12-13T09:40:45Z

apps/condo/pages/employee/[id]/index.tsx

+    if (error || loading || (!loading && !employee)) {
+        const errorToPrint = error ? ErrorMessage : (!loading && !employee) ? NotFoundMsg : ''
+        return <LoadingOrErrorPage title={loading ? LoadingInProgressMessage : errorToPrint} loading={loading} error={errorToPrint}/>
    }


Suggested change

if (error || loading || (!loading && !employee)) {

const errorToPrint = error ? ErrorMessage : (!loading && !employee) ? NotFoundMsg : ''

return <LoadingOrErrorPage title={loading ? LoadingInProgressMessage : errorToPrint} loading={loading} error={errorToPrint}/>

}

if (error || loading) {

const errorToPrint = error ? ErrorMessage : ''

return <LoadingOrErrorPage title={loading ? LoadingInProgressMessage : errorToPrint} loading={loading} error={errorToPrint}/>

}

if (!employee) {

return <LoadingOrErrorPage title={errorToPrint} loading={false} error={NotFoundMsg}/>

}

It's a little easier to read this way

Alllex202 · 2024-12-13T09:42:38Z

apps/condo/pages/employee/[id]/index.tsx

    }

    return (
        <EmployeePageContent
            employee={employeeWithSpecializations}
            updateEmployeeAction={updateEmployeeAction}
+            activeTicketsOrganizationEmployeeCount={activeTicketsOrganizationEmployeeCount?.meta?.count}


Suggested change

activeTicketsOrganizationEmployeeCount={activeTicketsOrganizationEmployeeCount?.meta?.count}

activeTicketsOrganizationEmployeeCount={activeTicketsOrganizationEmployeeCount?.meta?.count || 0}

You need to wait for the data to load also

Alllex202 · 2024-12-13T09:46:12Z

apps/callcenter

Shouldn't we provide the entire list of organizations for a call center request?

Alllex202 · 2024-12-13T09:48:39Z

apps/condo/pages/employee/[id]/index.tsx

    const { objs: organizationEmployeeSpecializations } = OrganizationEmployeeSpecialization.useObjects({
        where: {
            employee: { id: employeeId },
        },
    })

+    const { data: activeTicketsOrganizationEmployeeCount } = useGetOrganizationEmployeeTicketsCountForReassignQuery({


Your request is saved in the cache, so you should wait for the cache to load from local storage.
Let me remind you that the request to get the meta is cached for a minute.

wait persisor

const { persistor } = useCachePersistor() ... skip: !persistor || ...,

Alllex202 · 2024-12-13T09:53:21Z

apps/condo/domains/ticket/schema/Ticket.js

@@ -961,8 +964,10 @@ const Ticket = new GQLListSchema('Ticket', {
                ]
            )(...args)

-            /* NOTE: this sends different kinds of notifications on ticket create/update */
-            await sendTicketChangedNotifications.delay({ ticketId: updatedItem.id, existingItem, operation })
+            /* NOTE: this sends different kinds of notifications on ticket create/update except bulk update operation */


add please same comment on page

Alllex202 · 2024-12-13T09:57:17Z

apps/condo/domains/ticket/constants.js

@@ -55,6 +55,25 @@ const MAX_COMMENT_LENGTH = 700

 const DEFAULT_DEFERRED_DAYS = 30

+const TICKET_OTHER_SOURCE_ID = '7da1e3be-06ba-4c9e-bba6-f97f278ac6e4'


It's better to leave it in the test file, or just generate a random id.

Alllex202 · 2024-12-13T09:58:28Z

apps/condo/domains/ticket/constants.js

+ * updateTicketsForReassignmentEmployee - Query name in query/Ticket.graphql. Usage in DeleteEmployeeButtonWithReassignmentModal.jsx
+ */
+const DISABLE_PUSH_NOTIFICATION_FOR_OPERATIONS = [
+    'updateTickets',


We don't have this query

Suggested change

'updateTickets',

I need this query for tests. Frontend queries cannot be used in tests

I need this query for tests. Frontend queries cannot be used in tests

You can write the query you need for tests. You should to make sure that "updateTicketsForReassignmentEmployee" works correctly

Alllex202 · 2024-12-15T16:15:17Z

apps/condo/domains/organization/components/DeleteEmployeeButtonWithReassignmentModel.tsx

+    const employeeUserId = employee?.user?.id || null
+    const employeeOrganizationId = employee?.organization?.id || null
+
+    const getTicketReassignData = (ticket: Ticket) => {


This parameter is not of type Ticket. Use the correct generated type for your request (from @app/condo/gql)

Alllex202 · 2024-12-15T16:18:05Z

apps/condo/domains/organization/components/DeleteEmployeeButtonWithReassignmentModel.tsx

+        return resultObj
+    }
+
+    const getNotificationInfo = useCallback((notificationType: string, updatedTicketsCount = null) => {


Suggested change

const getNotificationInfo = useCallback((notificationType: string, updatedTicketsCount = null) => {

const getNotificationInfo = useCallback((notificationType: 'error' | 'warning' | 'success', updatedTicketsCount = null) => {

It would be better to specify specific values in the type

and then you won't need to put these values into constants.
(but if you want, you can leave them)

Alllex202 · 2024-12-15T16:20:03Z

apps/condo/domains/organization/components/DeleteEmployeeButtonWithReassignmentModel.tsx

+    }
+
+    const getNotificationInfo = useCallback((notificationType: string, updatedTicketsCount = null) => {
+        if (notificationType === ERROR_NOTIFICATION_TYPE) {


It would be better to use switch/case

Alllex202 · 2024-12-15T16:28:40Z

apps/condo/domains/organization/components/DeleteEmployeeButtonWithReassignmentModel.tsx

+            }
+        }
+
+        if (activeTicketsOrganizationEmployeeCount === updatedTicketsCount) {


do we really need this condition? If the number of tickets for an employee has changed up or down (not because of our feature), then an error will return, do we need this behavior?

Alllex202 · 2024-12-15T16:40:40Z

apps/condo/domains/ticket/constants.js

+ * updateTicketsForReassignmentEmployee - Query name in query/Ticket.graphql. Usage in DeleteEmployeeButtonWithReassignmentModal.jsx
+ */
+const DISABLE_PUSH_NOTIFICATION_FOR_OPERATIONS = [
+    'updateTickets',


I need this query for tests. Frontend queries cannot be used in tests

You can write the query you need for tests. You should to make sure that "updateTicketsForReassignmentEmployee" works correctly

Alllex202 · 2024-12-15T16:44:15Z

apps/callcenter

Check comments in cc pr

Alllex202 · 2024-12-15T16:54:49Z

apps/condo/pages/employee/[id]/index.tsx

-    }
+    const errorToPrint = error ? ErrorMessage : ''
+
+    if (error || loading || loadingTicketsOrganizationEmployeeCount) return <LoadingOrErrorPage title={loading ? LoadingInProgressMessage : errorToPrint} loading={loading} error={errorToPrint}/>


Wait persistor?

sonarqubecloud · 2024-12-16T06:57:07Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

pahaz · 2024-12-24T15:50:14Z

apps/condo/domains/ticket/schema/Ticket.test.js

+        })
+
+        // TODO: DOMA-10832 add check employee organization in Ticket access
+        test.skip('admin: cannot bulk update tickets executor field with user from different organization ', async () => {


pahaz · 2024-12-24T15:50:30Z

apps/condo/domains/ticket/schema/Ticket.test.js

+            })
+        })
+
+        test('admin: cannot bulk delete Tickets', async () => {


pahaz · 2024-12-24T15:58:42Z

apps/condo/domains/ticket/schema/Ticket.test.js

+            })
+        })
+
+        test('employee with role "canManageTickets": can bulk update Tickets', async () => {


bulk create test for employee with role "canManageTickets"

pahaz · 2024-12-24T15:59:01Z

apps/condo/domains/ticket/schema/Ticket.test.js

+            expect(get(updatedTicket2, ['assignee', 'id'], null)).toEqual(client.user.id)
+        })
+
+        test('employee without role "canManageTickets": cannot bulk update Tickets', async () => {


bulk create test for employee without role "canManageTickets"

pahaz · 2024-12-24T15:59:52Z

apps/condo/domains/ticket/schema/Ticket.test.js

+            })
+        })
+
+        test('organization "from" employee: can bulk update organization "to" tickets', async () => {


bulk create for organization "from" employee?

pahaz · 2024-12-24T16:24:19Z

apps/condo/domains/ticket/schema/Ticket.js

-            /* NOTE: this sends different kinds of notifications on ticket create/update */
-            await sendTicketChangedNotifications.delay({ ticketId: updatedItem.id, existingItem, operation })
+            /* NOTE: this sends different kinds of notifications on ticket create/update except bulk update operation */
+            if (!DISABLE_PUSH_NOTIFICATION_FOR_OPERATIONS.includes(get(context, ['req', 'body', 'operationName'], null))) {


It is impossible to assume that the logic of notifications will be tied to the name of the GQL mutation. This solution seems non-obvious and requires separate discussion.

tolmachev21 temporarily deployed to internal December 2, 2024 16:06 — with GitHub Actions Inactive

toplenboren reviewed Dec 3, 2024

View reviewed changes

nomerdvadcatpyat reviewed Dec 3, 2024

View reviewed changes

tolmachev21 temporarily deployed to internal December 3, 2024 17:37 — with GitHub Actions Inactive

Alllex202 requested changes Dec 4, 2024

View reviewed changes

tolmachev21 temporarily deployed to internal December 5, 2024 16:48 — with GitHub Actions Inactive

tolmachev21 temporarily deployed to internal December 9, 2024 03:41 — with GitHub Actions Inactive

tolmachev21 temporarily deployed to internal December 11, 2024 16:24 — with GitHub Actions Inactive

tolmachev21 force-pushed the feat/condo/DOMA-10516/reassign-employee-ticket branch from 7ce74a9 to 8885c69 Compare December 11, 2024 17:02

tolmachev21 temporarily deployed to internal December 11, 2024 17:02 — with GitHub Actions Inactive

tolmachev21 commented Dec 11, 2024

View reviewed changes

tolmachev21 requested review from Alllex202 and nomerdvadcatpyat December 11, 2024 17:31

tolmachev21 added the ✋🙂 Review please Comments are resolved, take a look, please label Dec 11, 2024

nomerdvadcatpyat reviewed Dec 12, 2024

View reviewed changes

tolmachev21 commented Dec 12, 2024

View reviewed changes

nomerdvadcatpyat approved these changes Dec 12, 2024

View reviewed changes

tolmachev21 temporarily deployed to internal December 12, 2024 15:03 — with GitHub Actions Inactive

tolmachev21 temporarily deployed to internal December 12, 2024 15:17 — with GitHub Actions Inactive

toplenboren reviewed Dec 12, 2024

View reviewed changes

tolmachev21 temporarily deployed to internal December 12, 2024 16:31 — with GitHub Actions Inactive

Alllex202 reviewed Dec 13, 2024

View reviewed changes

tolmachev21 added 10 commits December 14, 2024 12:52

feat(condo): DOMA-10516 delete unusage icons

1d5d49b

feat(condo): DOMA-10516 add new logic

774bc1e

feat(condo): DOMA-10516 add types in components

25c3c4f

feat(condo): DOMA-10516 change name and rewrite access

1b7713f

feat(condo): DOMA-10516 add more test cases and rewrite access logic

856ebca

feat(condo): DOMA-10516 remove redirent

5cfef78

feat(condo): DOMA-10516 add callcenter logic for this feature

d8a2549

feat(condo): DOMA-10516 add types

00d0910

feat(condo): DOMA-10516 fix review

c041916

feat(condo): DOMA-10516 update main

ec520ce

tolmachev21 force-pushed the feat/condo/DOMA-10516/reassign-employee-ticket branch from 132b856 to ec520ce Compare December 14, 2024 07:52

tolmachev21 temporarily deployed to internal December 14, 2024 07:52 — with GitHub Actions Inactive

feat(condo): DOMA-10516 add callcenter commit

4f76855

tolmachev21 temporarily deployed to internal December 14, 2024 08:28 — with GitHub Actions Inactive

Alllex202 reviewed Dec 15, 2024

View reviewed changes

feat(condo): DOMA-10516 wait persistor

cc611a1

tolmachev21 temporarily deployed to internal December 16, 2024 05:11 — with GitHub Actions Inactive

tolmachev21 added 2 commits December 16, 2024 11:50

feat(condo): DOMA-10516 wait persistor

a75f994

feat(condo): DOMA-10516 add commit from callcenter

4783488

tolmachev21 temporarily deployed to internal December 16, 2024 06:51 — with GitHub Actions Inactive

Alllex202 approved these changes Dec 16, 2024

View reviewed changes

tolmachev21 merged commit a5979e6 into main Dec 16, 2024
27 checks passed

tolmachev21 deleted the feat/condo/DOMA-10516/reassign-employee-ticket branch December 16, 2024 07:59

pahaz reviewed Dec 24, 2024

View reviewed changes

apps/condo/domains/ticket/schema/Ticket.test.js

})

})

test('admin: cannot bulk delete Tickets', async () => {

Copy link

Member

pahaz Dec 24, 2024

Choose a reason for hiding this comment

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

why?

pahaz reviewed Dec 24, 2024

View reviewed changes

		@@ -12,6 +12,7 @@ const {
		expectToThrowAuthenticationErrorToObjects,

		updateEmployeeAction={updateEmployeeAction}
		activeTicketsOrganizationEmployeeCount={get(activeTicketsOrganizationEmployeeCount, 'meta.count', 0)}

		message: notificationTitle('Ошибка'),
		description: notificationMessage('Повторите попытку'),


		const [notificationApi, contextHolder] = notification.useNotification()

		const filterTicket = `{"search":"${employeeName}", "status":["new_or_reopened","processing","completed","deferred"]}`

	const [updateTicketsWithReassignOrganizationEmployee] = useUpdateTicketsMutation()
	const [updateTickets] = useUpdateTicketsMutation()

	id_in: itemIds \|\| [itemId],
	id_in: isBulkRequest ? itemIds : [itemId],

	if (!isEmpty(propertyIds) && propertyIds.length !== originalInput.length) return false
	if (!isEmpty(propertyIds) return false


		return organizationId === get(property, 'organization')
		return isEqual(organizationIds, propertyOrganizationIds)

		const TICKET_OPEN_STATUS_ID = '6ef3abc4-022f-481b-90fb-8430345ebfc2'
		const TICKET_OTHER_SOURCE_ID = '7da1e3be-06ba-4c9e-bba6-f97f278ac6e4'

	query getOrganizationEmployeeTicketsCountForReassign ($userId: ID!, $organizationId: ID!) {
	query getOrganizationEmployeeTicketsCountForReassignment ($userId: ID!, $organizationId: ID!) {

		const notificationTitle = useCallback((notificationStatus: React.ReactNode) => <Typography.Text strong>{notificationStatus}</Typography.Text>, [])
		const notificationMessage = useCallback((notificationTitleLabel: React.ReactNode, updatedTicketsCount = null) => {

		@@ -55,6 +55,25 @@ const MAX_COMMENT_LENGTH = 700

		const DEFAULT_DEFERRED_DAYS = 30

		const TICKET_OTHER_SOURCE_ID = '7da1e3be-06ba-4c9e-bba6-f97f278ac6e4'

	employee: OrganizationEmployee
	employee: Pick<OrganizationEmployee, 'name'> & { organization?: Pick<OrganizationEmployee['organization'], 'id'>, user?: Pick<OrganizationEmployee['organization'], 'id'> }

		import { searchEmployeeUser } from '@condo/domains/ticket/utils/clientSchema/search'


		interface IDeleteEmployeeButtonWithReassignmentModel {

	interface IDeleteEmployeeButtonWithReassignmentModel {
	type DeleteEmployeeButtonWithReassignmentModelProps = {


		const [notificationApi, contextHolder] = notification.useNotification()

		const employeeUserId = employee?.user?.id

	const employeeOrganizationId = employee?.organization?.id
	const employeeOrganizationId = employee?.organization?.id \|\| null

	organizationId: employee?.organization?.id,
	organizationId: employee?.organization?.id \|\| null,

	const getNotificationInfo = useCallback((notificationType: string, updatedTicketsCount = null) => {
	const getNotificationInfo = useCallback((notificationType: 'error' \| 'warning' \| 'success', updatedTicketsCount = null) => {

feat(condo): DOMA-10516 reassign employee ticket #5563

feat(condo): DOMA-10516 reassign employee ticket #5563

Conversation

tolmachev21 commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alllex202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolmachev21 Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolmachev21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolmachev21 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alllex202 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolmachev21 Dec 11, 2024 •

edited

Loading

tolmachev21 Dec 12, 2024 •

edited

Loading

Alllex202 Dec 12, 2024 •

edited

Loading