Skip to content

ITEP-28186 Filter jobs by date #150

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

pplaskie
Copy link
Contributor

@pplaskie pplaskie commented May 5, 2025

📝 Description

Add possibility to filter jobs by date by select the range date.
Jobs are filtered by the creation date of the job.
PR also include basic test to check the functionality and refactor of function to create tabs in jobs-management-panel

JIRA: ITEP-28186

filteringbydate.mp4

✨ Type of Change

Select the type of change your PR introduces:

  • 🐞 Bug fix – Non-breaking change which fixes an issue
  • 🚀 New feature – Non-breaking change which adds functionality
  • 🔨 Refactor – Non-breaking change which refactors the code base
  • 💥 Breaking change – Changes that break existing functionality
  • 📚 Documentation update
  • 🔒 Security update
  • 🧪 Tests

🧪 Testing Scenarios

Describe how the changes were tested and how reviewers can test them too:

  • ✅ Tested manually
  • 🤖 Run automated end-to-end tests

✅ Checklist

Before submitting the PR, ensure the following:

  • 🔍 PR title is clear and descriptive
  • 📝 For internal contributors: If applicable, include the JIRA ticket number (e.g., ITEP-123456) in the PR title. Do not include full URLs
  • 💬 I have commented my code, especially in hard-to-understand areas
  • 📄 I have made corresponding changes to the documentation
  • ✅ I have added tests that prove my fix is effective or my feature works

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds the ability to filter jobs by a date range by updating the jobs dialog and underlying API filtering logic, while also including tests and a refactoring of the tabs creation in the jobs management panel.

  • Introduces date range filtering with new state management and UI components.
  • Refactors filtering state management and tab generation to improve clarity.
  • Adds new tests and a new date range picker component for manual date selection.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
web_ui/src/shared/components/header/jobs-management/jobs-dialog.component.tsx Added date filtering state, updated API call parameters, and refactored tab creation
web_ui/src/shared/components/date-range-picker-small/date-range-picker-small.test.tsx Added tests for the new date range picker functionality
web_ui/src/shared/components/date-range-picker-small/date-range-picker-small.component.tsx Introduced a new date range picker component with manual edition support
Comments suppressed due to low confidence (1)

web_ui/src/shared/components/header/jobs-management/jobs-dialog.component.tsx:90

  • Using JSON.stringify for comparing date ranges may lead to issues if the date serialization changes. Consider using a dedicated date comparison function or comparing each date component explicitly.
const isInitialRange = useMemo(() => JSON.stringify(range) === JSON.stringify(INITIAL_DATES), [range, INITIAL_DATES]);

@jpggvilaca
Copy link
Contributor

Screenshot 2025-05-06 at 11 27 29
I noticed infinite renders when i open the jobs dialog. Could it have to do with the miliseconds of the date? We should make sure we only compare days and nothing else

@pplaskie
Copy link
Contributor Author

pplaskie commented May 6, 2025

Screenshot 2025-05-06 at 11 27 29 I noticed infinite renders when i open the jobs dialog. Could it have to do with the miliseconds of the date? We should make sure we only compare days and nothing else

While checking I remembered myself that it should be because of the jobs polling.

@camiloHimura
Copy link
Contributor

@pplaskie Can you add a video/screenshot of the new workflow?

@pplaskie
Copy link
Contributor Author

pplaskie commented May 6, 2025

@pplaskie Can you add a video/screenshot of the new workflow?

Sure!

pplaskie added 4 commits May 14, 2025 08:43
* condition to not use header in date range picker
* remove useEffect and make changes accordingly
* remove unnecessary comment
* add comment about exclusivness of filtering
@jpggvilaca jpggvilaca force-pushed the pplaskie/ITEP-28186_filter_jobs_by_date branch from b4330c6 to 3ca5fe3 Compare May 14, 2025 06:44
@@ -0,0 +1,93 @@
// Copyright (C) 2022-2025 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

We should put this component close to jobs-actions because it's needed only there. If we need this component later in other places, we will move it to @geti/ui :).


const mockOnChange = jest.fn();

const renderComponent = (props = {}) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

np. props can be removed, we don't use them.

it('should open the date range dialog when the button is clicked', async () => {
renderComponent();

expect(screen.getByRole('button', { name: 'Select date range' })).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

np. TBH we don't need to check if the button is in the document because if it is not then userEvent.click in line 41 will break.


import { QuietActionButton } from '../quiet-button/quiet-action-button.component';

interface DateRangePickerSmall extends RangeCalendarProps<DateValue> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We allow consumer to pass focusedValue and onFocusChange which will never be used because we use internal state for this. We should do Omit<RangeCalendarProps<DateValue>, 'focusedValue' | 'onFocusChange>;`

Comment on lines -107 to -120
useEffect(() => {
if (isLoadingUsers) return;

const isSelectedUserInProject: boolean | undefined = users?.some(hasEqualId(selectedUser));

if (!!isSelectedUserInProject) return;

setSelectedUser(undefined);
onChange(selectedProject, undefined, selectedJobTypes);
}, [isLoadingUsers, onChange, selectedJobTypes, selectedProject, selectedUser, users]);

useEffect(() => {
onChange(selectedProject, selectedUser, selectedJobTypes);
}, [selectedProject, selectedUser, selectedJobTypes, onChange]);
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

const [projectIdFilter, setProjectIdFilter] = useState<string | undefined>();
const [userIdFilter, setUserIdFilter] = useState<string | undefined>();
const [jobTypeFilter, setJobTypeFilter] = useState<JobType[]>([]);
const TODAY = today(getLocalTimeZone());
Copy link
Contributor

@dwesolow dwesolow May 16, 2025

Choose a reason for hiding this comment

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

TODAY is not memoized and probably recreates on each render causing INITIAL_DATES to recreate. WDYT of defining INITIAL_DATES inside useState leveraging the callback, i.e. useState<RangeValue<DateValue>>(() => { define and return initial dates }), the bonus of that approach is that we could get rid of useMemo that does not bring value in this case.

Edit: I see INITIAL_DATES and TODAY are used below in the code. I think we could safely remove useMemo unless you noticed any issues with the performance so u added that.

const { organizationId, workspaceId } = useWorkspaceIdentifier();

const { projectIdFilter, userIdFilter, jobTypeFilter } = defaultValues;
const [jobFilters, setJobFilters] = useState<FiltersType>(values);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this state here? I see we define state in the jobs-dialog and we have access to that state here. IMO we could remove the jobFilters from this component.

[range, INITIAL_DATES]
);

const areFiltersChanged = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need useMemo here, all values are primitives and it's quite fast todo comparison of booleans.

@@ -60,193 +87,101 @@ export const JobsDialog = ({ isFullScreen, onClose, setIsFullScreen }: JobsDialo
}
);

const allJobs = getAllJobs(data);
const isInitialRange = useMemo(
() => JSON.stringify(range) === JSON.stringify(INITIAL_DATES),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a trick not to do range.end == INITIAL_DATES.end etc?

Copy link
Contributor Author

@pplaskie pplaskie May 19, 2025

Choose a reason for hiding this comment

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

yes, I'm changing to isEqual from lodash

import { JobCount } from '../../../../core/jobs/jobs.interface';
import { CornerIndicator } from '../../../../pages/annotator/annotation/annotation-filter/corner-indicator.component';
import { RefreshButton } from '../../../../pages/annotator/components/sidebar/dataset/refresh-button/refresh-button.component';
Copy link
Contributor

Choose a reason for hiding this comment

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

This button comes from the annotator feature. Should we consider moving the button to the shared or the the @geti/ui?

return;
}

onChange && onChange({ ...range, [attribute]: value });
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onChange && onChange({ ...range, [attribute]: value });
isFunction(onChange) && onChange({ ...range, [attribute]: value });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants