-
Notifications
You must be signed in to change notification settings - Fork 1
Fix: add period dates and update data entry #505
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
Fix: add period dates and update data entry #505
Conversation
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.
[code review]. Looks good, just two minor observations. Thanks @gqcorneby
src/domain/entities/DatePeriod.ts
Outdated
return []; | ||
} | ||
|
||
const allStartTimes = _(periods) |
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.
I think we can extract this functionality into a private method and reuse it for start/end Times.
_(periods)
.compactMap(p => stringToTime(p.startDate))
.value();
Also I think the name of the variable should be allEndTimeS
to be consistent with allStartTimes
, right?
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.
Thanks, Eduardo! Minor thing but I went back and forth with this 😆 I thought it might be overkill to extract it as a method. Will apply 👍
src/utils/date.ts
Outdated
0 | ||
).getDate(); | ||
const dayFraction = (dateBDay - dateADay) / daysInCurrentMonth; | ||
monthDiff += dayFraction; |
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.
Not a big deal, but normally we try to avoid mutating values. We can directly call to return monthDiff + dayFraction
Also I think the condition should be if (dateA.getTime() !== dateB.getTime())
to compare the actual dates and not the reference, right?
Example:
var a = new Date("2024-12-05");
var b = new Date("2024-12-05");
console.log(a !== b) // true
console.log(a.getTime() !== b.getTime()) // false
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.
Good catch! Thank you!
📌 References
📝 Implementation
dataInputPeriod
is generated based on attributeGL_INTERVAL_DATES (Data Input Dates)
GL_DATASET_PERIOD_DATES (Period Dates)
openFuturePeriods
when selecting project or updating data input datesGL_DATASET_PERIOD_DATES
toISODateWithoutTimezone
when savingnew Date(2025, 3, 1, 0, 0, 0) April 1 2025 00:00:00 GMT+8
toISOString()
then get the date value, which will beMarch 31 2025
new Date(2025, 3, 1, 0, 0, 0).toISOString()
is2025-03-31T16:00:00.000Z
TODO:
📹 Screenshots/Screen capture
CREATE DATA SET
2025-09-03.17-39-43.mp4
UPDATE 2027 CLOSING (fields disable fields)
2025-09-03.17-46-44.trimmed.mp4
2025-09-03.18-02-31.mp4
🔥 Notes to the tester