Add date filter #747
Add date filter #747
Conversation
|
Ref: #748 |
|
@yaacov |
| @@ -192,6 +192,7 @@ const cellCreator: Record<string, (props: CellProps) => JSX.Element> = { | |||
| <VirtualMachineIcon /> {value} | |||
| </RouterLink> | |||
| ), | |||
| [C.MIGRATION_STARTED]: ({ value }: CellProps) => <>{value}</>, | |||
There was a problem hiding this comment.
can you add a small helper to normalize the output ?
here an example:
/**
* Converts a given ISO date string in a known format and timezone to a UTC ISO string.
*
* @param {string} isoDateString - The ISO date string in a known format and timezone.
* @returns {string} The equivalent UTC ISO string.
*/
function convertToUTC(isoDateString: string): string {
const date = new Date(isoDateString);
// Convert the parsed date to a UTC ISO string
const utcIsoDate = date.toISOString();
return utcIsoDate;
}There was a problem hiding this comment.
adding note about string date format, for future reference
Ref: https://www.ietf.org/rfc/rfc3339.txt
| @@ -96,6 +96,11 @@ const groupedEnumMatcher = { | |||
| matchValue: enumMatcher.matchValue, | |||
| }; | |||
|
|
|||
| const dateMatcher = { | |||
| filterType: 'date', | |||
| matchValue: freetextMatcher.matchValue, | |||
There was a problem hiding this comment.
ok, so this is not a "from date" but an "at date" filter ... but you need to adjust for timezone and UTC ...
a way to do it is to get the datetime of 00 and 24 of the day, maybe using a helper method:
/**
* Given an ISO date string, returns two epoch timestamps:
* one for the start (00:00:00 UTC) and one for the end (23:59:59 UTC) of the UTC day of the date in the given ISO string.
*/
function getStartAndEndEpochs(isoDateString: string): [number, number] {
const date = new Date(isoDateString);
date.setUTCHours(0, 0, 0, 0);
const startEpoch = date.getTime();
date.setUTCHours(23, 59, 59, 999);
const endEpoch = date.getTime();
return [startEpoch, endEpoch];
}and then check the date you have is between this timestamps
/**
* Checks if a given date string falls within the start and end epoch times of another provided date string.
*
* @param {string} referenceIsoDate - The reference ISO date string to determine the UTC day.
* @param {string} targetIsoDate - The ISO date string to check if it falls within the determined UTC day.
* @returns {boolean} - True if the targetIsoDate falls within the UTC day of the referenceIsoDate, false otherwise.
*/
function isDateWithinDay(referenceIsoDate: string, targetIsoDate: string): boolean {
const [startEpoch, endEpoch] = getStartAndEndEpochs(referenceIsoDate);
const targetEpoch = new Date(targetIsoDate).getTime();
return targetEpoch >= startEpoch && targetEpoch <= endEpoch;
}There was a problem hiding this comment.
used luxon lib to perform the calculations
There was a problem hiding this comment.
Your implementation with loxun.hasSame addresses the "is it this day" scenario effectively. However, it might not cater to the broader requirements of handling date ranges and date-time ranges. My initial example using the internal Date object's getTime was to demonstrate an approach that can be easily extended for both single day checks and date range evaluations. Could we consider adapting the implementation to ensure it's versatile for all these use cases?
about using external the library luxon, see:
#747 (comment)
There was a problem hiding this comment.
Your implementation with loxun.hasSame addresses the "is it this day" scenario effectively. However, it might not cater to the broader requirements of handling date ranges and date-time ranges.
Please check the matcher implementation in PR #754 :
https://github.com/rszwajko/forklift-console-plugin/blob/7f99ef49ccafedd287790fe45cd96231a612d8bf/packages/common/src/components/FilterGroup/matchers.ts#L116
The line demonstrates parsing ISO time internal format and checking if date belongs to the interval in one line.
Links:
https://moment.github.io/luxon/api-docs/index.html#intervalfromiso
https://en.wikipedia.org/wiki/ISO_8601#Time_intervals
My initial example using the internal Date object's getTime was to demonstrate an approach that can be easily extended for both single day checks and date range evaluations.
Please create a PR with your code and describe what is the added value, what functionality is missing in Luxon and why we should switch.
There was a problem hiding this comment.
Please create a PR with your code and describe what is the added value, what functionality is missing in Luxon and why we should switch.
I appreciate your suggestion. However, our initial agreement was to proceed using the internal date object. It would be helpful if we could stick to that plan and move forward as discussed. Thank you for understanding.
|
looks good 👍 |
| @@ -110,8 +111,8 @@ export const mergeData = ( | |||
| !vm.error && | |||
| !vm.conditions?.find((condition) => condition.type === 'Canceled'), | |||
| ).length || 0, | |||
| migrationCompleted: migration?.completed, | |||
| migrationStarted: migration?.started, | |||
| migrationCompleted: convertToUTC(migration?.completed), | |||
There was a problem hiding this comment.
we don't do that in other places, we don't need to do it here,
you make sure we use UTC here:
https://github.com/kubev2v/forklift-console-plugin/pull/747/files#diff-5c809c708698ad27265c0c3a2e2970700e439c4d826a6e4b651a67c4d8499a31R103
so no, need to do it twice
There was a problem hiding this comment.
- the matcher is a re-usable component which additionally gets input from the user (the filter). That's why we need to repeat the conversions.
- Plans screen follows the "flat" design - the data is converted once and then used across UI. Since we decided to use UTC this is the right place to put the transformation. Note that we also perform validation here - invalid dates are discarded.
There was a problem hiding this comment.
the matcher is a re-usable component which additionally gets input from the user (the filter). That's why we need to repeat the conversions.
This is the reason it's in the matcher, and I agree we should do a conversion in the matcher.
Here on the other hand it's wrong, it removed information we may want, e.g. the timezone.
We agreed to show users UTC when creating the new date filter, not to change the way we store data internally.
This file is not part of the date filter, so if we want to change we can do it in a different PR, and IMHO we have no need to change the internal data, even in anther PR.
Plans screen follows the "flat" design
right, that will change in 2.6, but even using "flat" design we were fine with the data as is, since this code is not directly connected to the filter, I prefer to not touch it in this PR
There was a problem hiding this comment.
This is the reason it's in the matcher, and I agree we should do a conversion in the matcher.
Here on the other hand it's wrong, it removed information we may want, e.g. the timezone.
The idea of the flattening approach was to perform data transformations once and let the "dumb" components (i.e. table cells) consume the results directly. If we want to support only UTC then this is the best place to put the logic.
Otherwise each component needs to remember to do the conversion.
Plans screen follows the "flat" design
right, that will change in 2.6, but even using "flat" design we were fine with the data as is, since this code is not directly connected to the filter, I prefer to not touch it in this PR
The main reason of adding the column was to demo the filter (we have no other field to test it).
There was a problem hiding this comment.
The main reason of adding the column was to demo the filter (we have no other field to test it).
You are removing migrationStarted and changing migrationCompleted
I don't see an added column, am i missing something ?
My question is, why do you change migrationCompleted , AFAIU the demo of the date filter will work just the same with or without this change ?
p.s.
IFAIU the date and time data is always encapsulated in a component that will show it in the correct way,
once inside the filter, that does not show the data directly, and in the second place it's used as data to Timestamp component that expect the unmodified data, see:
https://github.com/openshift/console/blob/5002947c029ad1dfdc1b0dc88275e8dea5670cf7/frontend/public/components/utils/details-page.tsx#L155
There was a problem hiding this comment.
You are removing migrationStarted and changing migrationCompleted
Not removing but converting both migrationStarted and migrationStarted to UTC.
I don't see an added column, am i missing something ?
Please check changes in the PlansPage -> https://github.com/kubev2v/forklift-console-plugin/pull/747/files#diff-e7a795084a21e23e501a782f72e5e5f90ba86bc27e9facbb1310fbce3d443b86
My question is, why do you change migrationCompleted , AFAIU the demo of the date filter will work just the same with or without this change ?
As you requested the UI should be UTC based so converting both values is justified.
IFAIU the date and time data is always encapsulated in a component that will show it in the correct way,
once inside the filter, that does not show the data directly, and in the second place it's used as data to Timestamp component that expect the unmodified data
Timestamp component will display any date. The objective was to display the date in UTC. This could be accomplished by converting in the component layer (in the table cell) or in the data transform layer.
As already pointed above the advantage of "flat" approach is that the data is pre-calculated once. This is used for other props as well i.e. check the vmDone prop:
vmDone:
migration?.vms?.filter(
(vm) =>
!!vm.completed &&
!vm.error &&
!vm.conditions?.find((condition) => condition.type === 'Canceled'),
).length || 0,
There was a problem hiding this comment.
Our primary focus for this PR is creating the date filter component. To maintain clarity and focus, could we consider reverting the unrelated changes? This would help streamline our review and ensure alignment with the main objective. I appreciate your attention to this matter and your collaboration.
There was a problem hiding this comment.
OK. If you prefer you prefer we can revert changes to migrationCompleted. Is there any other part that you would like to revert?
There was a problem hiding this comment.
Is there any other part that you would like to revert?
I appreciate your input. For this particular instance, there's no need to make the changes you suggested in lines 113 and 114. Thank you for your understanding and cooperation.
| const dateMatcher = { | ||
| filterType: 'date', | ||
| matchValue: (value: string) => (filter: string) => | ||
| DateTime.fromISO(value).toUTC().hasSame(DateTime.fromISO(filter).toUTC(), 'day'), |
There was a problem hiding this comment.
p.s.
the next task is from .. to it makes sense to use '>=' over hasSame
using >= as the added benefit of using standards, JS Date is standard while luxon is implementing none standard date formats
There was a problem hiding this comment.
Date range filter will use a different matcher but I do plan to use luxon there as well.
IMHO this is the standard approach.
There was a problem hiding this comment.
I do plan to use luxon there as well.
that is actually a "task completion definition", i explicitly requested to implement the filter without using external libraries, unless it's very hard to implement using vanilla type script, I explained that I would like to remove the requirement if possible.
the rule of thumb was that if implementing the filter will take a lot of code more using vanilla type script, then it is ok to use a library. In this case it's possible to create a method that can be reusable between all the date-time watchers and will actually use less lines of code and be more readable when using vanilla typescript, see examples above, please drop the requirement on the external library in this case.
There was a problem hiding this comment.
The code you proposed is interesting but cannot compete with an established library in terms of reliability and readability. I understand there are some other benefits of this approach. Let us discuss this later on.
There was a problem hiding this comment.
The code you proposed is interesting but cannot compete with an established library in terms of reliability and readability.
hmm, Date is a library that is part of the core of java script, it's reliable and readable ( depending on the writer :-) )
Let us discuss this later on.
Discussion is always appreciated, for this PR, please don't use the library.
There was a problem hiding this comment.
OK. Let's put in on hold until we clear this issue.
Allow to filter events at given date. Key points: 1. multiple dates can be choosen 2. ISO date format used (YYYY-MM-DD) regardless of the locale 3. date comparison is done in UTC zone 4. Migration started column (timestamp) added to Plans table Reference-Url: https://github.com/oVirt/ovirt-web-ui/blob/ea9a10e75e7dc965ddc6a1a7f2d36f6301117bbc/src/components/Toolbar/DatePickerFilter.js Signed-off-by: Radoslaw Szwajkowski <[email protected]>
There was a problem hiding this comment.
As agreed upon during our issue design discussions, let's prioritize using JavaScript's native Date object over Luxon for this implementation. This approach will align with our previous decisions and ensure consistency across our codebase. Please ensure that the methods and utilities we design are compatible with the Date object functionalities. Thank you for your attention to this detail, and let's maintain our alignment with the decisions made during the design phase.
The agreement was to use tool suitable for the task. Luxon is quite suitable:)
We already use luxon across the UI so this implementation is consistent with the rest of the code. |
I understand your perspective on Luxon, and while I acknowledge its usefulness, our initial agreement prioritized using the internal Date object unless its implementation required excessive coding. I'd appreciate it if we could stick to that plan. Thank you for your understanding. |
Drop converting migrationStarted and migrationCompleted to UTC in the data transformation layer. Signed-off-by: Radoslaw Szwajkowski <[email protected]>
Signed-off-by: Radoslaw Szwajkowski <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
|
@yaacov |
| @@ -23,3 +23,7 @@ export const BlueInfoCircleIcon = () => <div data-test-element-name="BlueInfoCir | |||
| export const useModal = (props) => <div data-test-element-name="useModal" {...props} />; | |||
| export const ActionService = () => <div data-test-element-name="ActionService" />; | |||
| export const ActionServiceProvider = () => <div data-test-element-name="ActionServiceProvider" />; | |||
|
|
|||
| export const Timestamp = ({ timestamp }: TimestampProps) => ( | |||
Allow to filter events at given date.
Key points:
Reference-Url: https://github.com/oVirt/ovirt-web-ui/blob/ea9a10e75e7dc965ddc6a1a7f2d36f6301117bbc/src/components/Toolbar/DatePickerFilter.js