Skip to content
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(condo): DOMA-8523 added filter for expired tickets. Added saving previous query params for pages #4571

Merged
merged 37 commits into from Apr 11, 2024

Conversation

Alllex202
Copy link
Contributor

@Alllex202 Alllex202 commented Apr 3, 2024

  1. Added filter for expired tickets - to filter by expired tickets that are completed, the isCompletedAfterDeadline field was added
  2. Added saving previous query params for pages (This behavior has been updated only in condo and callcenter apps. Miniapps will be updated in another pr)
  3. Updated filter layouts
  4. Updated search behavior in multi-selects - now the search is not reset when selecting an item

And little demo videos

Added filter for expired tickets and saving previous query params for pages

Before

2024-04-04.15.38.00.mov

After

2024-04-04.15.35.39.mov
Снимок экрана 2024-04-04 в 15 40 46

Updated search behavior in multi-selects

Before

2024-04-04.15.38.36.mov

After

2024-04-04.15.36.44.mov

@Alllex202 Alllex202 force-pushed the feat/condo/DOMA-8523/update-filters branch from a8227c8 to 475c0bf Compare April 4, 2024 08:39
@Alllex202 Alllex202 added 👶 small Easy to review changes up to 50 lines of code ✋🙂 Review please Comments are resolved, take a look, please 🚨 Migrations We have a database migrations here! labels Apr 4, 2024
}
}, [checkboxValueFromQuery])

return [value, handleChangeCheckbox, handleResetCheckboxWithoutUpdateQuery, handleChangeCheckboxWithoutUpdateQuery]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe return a object?
It's not clear what returns from hook without read the hook code

Comment on lines 802 to 803
const reduceNonEmpty = (cnt, filter) => cnt + Number((typeof filters[filter] === 'string' || Array.isArray(filters[filter])) && filters[filter].length > 0)
const appliedFiltersCount = Object.keys(filters).reduce(reduceNonEmpty, 0)
Copy link
Member

Choose a reason for hiding this comment

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

memo?

apps/condo/domains/ticket/schema/Ticket.js Show resolved Hide resolved
Comment on lines 53 to 63
OR: [
{ AND: { status: { type_in: [COMPLETED_STATUS_TYPE, CLOSED_STATUS_TYPE] }, isCompletedAfterDeadline: true } },
{ AND: { status: { type_in: [NEW_OR_REOPENED_STATUS_TYPE, PROCESSING_STATUS_TYPE] }, deadline_lt: (new Date()).toISOString() } },
],
Copy link
Member

Choose a reason for hiding this comment

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

What about deffered status?

const canManageEmployee = get(link, 'role.canInviteNewOrganizationEmployees', null)
const employeeId = get(link, 'id')

usePreviousQueryParams({ trackedParamNames: ['sort', 'filters'], employeeId })
Copy link
Member

Choose a reason for hiding this comment

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

Behavior with saving filters is needed on all pages with filters?

Mb move it to some Context, use it in _app and not not copy paste this logic everywhere where we need filters?

type usePreviousQueryParamsType = (props: { trackedParamNames: Array<string>, delimitersParamNames?: Array<string>, employeeId?: string }) => void

export const usePreviousQueryParams: usePreviousQueryParamsType = ({
delimitersParamNames,
Copy link
Member

Choose a reason for hiding this comment

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

It’s not clear from the name of the argument what it’s responsible for.
Mb tab or some other name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't think of a good name for this variable. Changed it to paramNamesForPageChange and added a docs

@Alllex202 Alllex202 force-pushed the feat/condo/DOMA-8523/update-filters branch from 475c0bf to eab2c0a Compare April 5, 2024 17:35
@Alllex202 Alllex202 added 🔬 WIP Not intended to be merged right now, it is a work in progress and removed 👶 small Easy to review changes up to 50 lines of code ✋🙂 Review please Comments are resolved, take a look, please labels Apr 5, 2024
Comment on lines 135 to 145
// // Case without tabs
// useEffect(() => {
// if (!employeeId) return
// if (withDelimitersParams) return
// applyQueryParamsFromLocalStorage()
// }, [employeeId])
// useDeepCompareEffect(() => {
// if (!employeeId) return
// if (withDelimitersParams) return
// saveQueryParamsToLocalStorage()
// }, [employeeId, trackedParams])
Copy link
Member

Choose a reason for hiding this comment

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

why you comment it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted this. Was updated logic

export const usePreviousQueryParams: usePreviousQueryParamsType = ({
delimitersParamNames,
employeeId,
trackedParamNames,
Copy link
Member

Choose a reason for hiding this comment

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

Ok
Also you can wrap this hook in less common hook, specified to save filters logic and use default params there

@@ -775,16 +785,16 @@ const Ticket = new GQLListSchema('Ticket', {
if (resolvedStatusId) {
calculateTicketOrder(resolvedData, resolvedStatusId)

const existedStatusId = get(existingItem, 'status', null)
const resolvedStatus = await TicketStatus.getOne(context, { id: resolvedStatusId })
Copy link
Member

Choose a reason for hiding this comment

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

can we use here getById?
And below in existedStatus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try

expect(updatedTicket8).toHaveProperty('isCompletedAfterDeadline', true)
})

test('Should be auto-set false if status changed to "completed", "closed" or "closed" before deadline', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test('Should be auto-set false if status changed to "completed", "closed" or "closed" before deadline', async () => {
test('Should be auto-set false if status changed to "completed", "closed" or "canceled" before deadline', async () => {

let stopPoint

if (nextStatusId === STATUS_IDS.DECLINED) {
if ((prevStatusId === STATUS_IDS.COMPLETED || prevStatusId === STATUS_IDS.CLOSED || nextStatusId === STATUS_IDS.DECLINED) && completedAt) {
Copy link
Member

Choose a reason for hiding this comment

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

here you check nextStatusId === STATUS_IDS.DECLINED twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

} else if (nextStatusId === STATUS_IDS.COMPLETED) {
stopPoint = completedAt
} else if (nextStatusId === STATUS_IDS.CLOSED) {
if ((prevStatusId === STATUS_IDS.COMPLETED || nextStatusId === STATUS_IDS.CLOSED) && completedAt) {
Copy link
Member

Choose a reason for hiding this comment

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

also for nextStatusId === STATUS_IDS.CLOSED

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Alllex202 Alllex202 added ✋🙂 Review please Comments are resolved, take a look, please and removed 🔬 WIP Not intended to be merged right now, it is a work in progress labels Apr 8, 2024
@Alllex202 Alllex202 force-pushed the feat/condo/DOMA-8523/update-filters branch from 2ffd950 to 52fe86b Compare April 9, 2024 10:10
@Alllex202 Alllex202 force-pushed the feat/condo/DOMA-8523/update-filters branch from 52fe86b to 3691a44 Compare April 11, 2024 15:59
Copy link

sonarcloud bot commented Apr 11, 2024

Quality Gate Passed Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.7% Duplication on New Code

See analysis details on SonarCloud

@Alllex202 Alllex202 merged commit ceb7d48 into master Apr 11, 2024
25 checks passed
@Alllex202 Alllex202 deleted the feat/condo/DOMA-8523/update-filters branch April 11, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 Migrations We have a database migrations here! ✋🙂 Review please Comments are resolved, take a look, please
Development

Successfully merging this pull request may close these issues.

None yet

2 participants