-
Notifications
You must be signed in to change notification settings - Fork 24
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: [DHIS2-18310] enable non-Gregorian calendars in views & lists & forms #3900
base: master
Are you sure you want to change the base?
feat: [DHIS2-18310] enable non-Gregorian calendars in views & lists & forms #3900
Conversation
8983fd7
to
0294f8d
Compare
src/core_modules/capture-core/components/FiltersForTypes/Date/DateFilter.component.js
Outdated
Show resolved
Hide resolved
src/core_modules/capture-core/components/FiltersForTypes/Date/DateFilter.component.js
Outdated
Show resolved
Hide resolved
src/core_modules/capture-core/components/FiltersForTypes/Date/DateFilter.component.js
Outdated
Show resolved
Hide resolved
Hi @alaa-yahia,
Screen.Recording.2024-12-12.at.16.51.40.movIn the video above I used the Ethiopian calendar. Can you have a look? Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good start, but some things we will have to work a bit more on.
In addition to the individual comments we should convert the "last updated" tooltips from ISO to local calendar (I saw this in both the stages and events Widget and the enrollment Widget). Had a very quick look at the stages and events Widget and for now I think you can use something like moment(fromServerDate(updatedAt)).toISOString()
and put the result into the clientToView converter of type DATETIME (we should probably also use the serverToClient converter, but this will be checked and resolved in separate ticket)
src/core_modules/capture-core/components/FiltersForTypes/Date/DateFilter.component.js
Outdated
Show resolved
Hide resolved
src/core_modules/capture-core/components/D2Form/field/configs/dateField/getDateFieldConfig.js
Outdated
Show resolved
Hide resolved
…ent date (#3905) feat: typing the date when editing enrollment and incident date
da9bf63
to
013db2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to other comments:
- We should fix the logic in
getDateRangeValidator
andgetDateTimeRangeValidator
. These validators are being used in the search form when using tracked entity attributes of value type date and dateTime. We should replace the parseDate logic and leverage the validation done in the ui library component. - Don't use parseDate on local dates. I think this means that we should remove parseDate from the codebase in its entirety (I didn't see any valid reason to use it anymore, but let me know if I missed something)
const years = now.diff(age, 'years'); | ||
age.add(years, 'years'); | ||
function getCalculatedValues(dateValue: ?string): AgeValues { | ||
const calendar = systemSettingsStore.get().calendar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One architectural principle that we have been trying to adhere to is that the capture-ui could be extracted and used in any app. I.e., it shouldn't retrieve data from app specific objects like we do here. Can we instead of this, pass in calendarType
to the component?
@@ -10,6 +12,7 @@ import { AgeDateInput } from '../internal/AgeInput/AgeDateInput.component'; | |||
import defaultClasses from './ageField.module.css'; | |||
import { orientations } from '../constants/orientations.const'; | |||
import { withInternalChangeHandler } from '../HOC/withInternalChangeHandler'; | |||
import { convertStringToTemporal, convertTemporalToString } from '../../capture-core/utils/converters/date'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the reasons mentioned in the comment on line 57; can we put these helper functions in capture-core-utils
(capture-core-utils/date
is probably fine for now) and pass in the calendarType
and dateFormat
instead of getting these values from systemSettingsStore
internally in the functions? If needed, we can also create helper functions in the location where the helper functions currently are found, but these helpers shouldn't be invoked from files in capture-ui
.
const momentDate = moment(localDate); | ||
if (!momentDate.isValid()) { | ||
return ''; | ||
} | ||
|
||
const formattedIsoDate = momentDate.format('YYYY-MM-DD'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't use moment on our input here since the input is in the local calendar type.
@@ -36,7 +39,7 @@ function convertDateTime(formValue: DateTimeValue): ?string { | |||
function convertDate(dateValue: string) { | |||
const parsedDate = parseDate(dateValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inside the parseDate function, the input date is being validated using moment and we shouldn't do that since the input is in the local calendar type. Looking into this a bit, there should be no need to validate the date in the converter here as this should be handled elsewhere. I.e., you can assume the date is valid at this point. So, using something like convertStringToTemporal
and then passing the ISOstring of the returned Temporal to convertLocalToIsoCalendar
could be the way to go (or alternatively convertLocalToIsoCalendar
could handle the 'dd-mm-yyyy' format).
We should look into the dateTime converter as well and apply the same idea there.
const parseData = dateValue && onParseDate(dateValue); | ||
|
||
function getCalculatedValues(dateValue: ?string): AgeValues { | ||
const parseData = dateValue && parseDate(dateValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use the parseDate function as is because it is using moment internally (and we are passing in a date from a local calendar type). Let's use the following approach here:
- In
handleDateBlur
in this file, let's make sure we check the validation that is passed in from the component and only invokegetCalculatedVaules
if the date is valid (currently, the app crashing sometimes with invalid dates). The validation executed in the component is the source of truth for whether the date is valid or not. - Don't use parseDate here. After point .1, the value passed in to
getCalculatedValues
should always be valid and you should be able to useconvertStringToTemporal
directly.
In general, we should replace parseDate in our codebase if it is being used on dates of local calendar types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alaa-yahia, I found a few bugs that I hope you can look into. I tested with the Ethiopian and Nepalese calendars. Thank you!
const years = now.diff(age, 'years'); | ||
age.add(years, 'years'); | ||
function getCalculatedValues(dateValue: ?string, calendarType: ?string, dateFormat: ?string): AgeValues { | ||
const now = Temporal.Now.plainDateISO().withCalendar(calendarType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -12,6 +12,8 @@ import { statusTypes } from 'capture-core/events/statusTypes'; | |||
import { NonBundledDhis2Icon } from '../../../../NonBundledDhis2Icon'; | |||
import type { Props } from './stageOverview.types'; | |||
import { isEventOverdue } from '../StageDetail/hooks/helpers'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -5,21 +5,20 @@ import i18n from '@dhis2/d2-i18n'; | |||
import { Tag } from '@dhis2/ui'; | |||
import { PreviewImage } from 'capture-ui'; | |||
import { dataElementTypes, type DataElement } from '../metaData'; | |||
import { convertMomentToDateFormatString } from '../utils/converters/date'; | |||
import { convertIsoToLocalCalendar } from '../utils/converters/date'; | |||
import { stringifyNumber } from './common/stringifyNumber'; | |||
import { MinimalCoordinates } from '../components/MinimalCoordinates'; | |||
import { TooltipOrgUnit } from '../components/Tooltips/TooltipOrgUnit'; | |||
|
|||
function convertDateForListDisplay(rawValue: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the search page, the currentSearchTerms are not converted correctly. I think the problem arises when using the clientToList
converter in the SearchResultsHeader.component. In the screenshot bellow, the label Birth date: 05-05-2017 -> 07-05-2017
should be displayed.
<DateField | ||
width={200} | ||
value={selectedDate} | ||
onBlur={dateChangeHandler} | ||
label={dateLabel} | ||
date={selectedDate} | ||
dense | ||
locale={locale} | ||
onDateSelect={dateChangeHandler} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calendar for the enrollment date and incident date in the WidgetEnrollment component is in ISO format. After selecting a date, the value in the input field is also displayed in ISO format.
Screen.Recording.2025-01-14.at.10.58.47.mov
…non-gregorian-calendars-in-views-and-lists
@simonadomnisoru I've pushed bug fixes addressing your feedbacks. Could you take another look when you have a time? |
Implements DHIS2-18310 & DHIS2-18311 & DHIS2-18309