add period dates action. script to migrate existing data #478
Conversation
|
@tokland I merged this PR in a rush but, whenever you have time, I would like to provide a code review anyway and we can have a different PR with the changes if needed. Thanks! |
There was a problem hiding this comment.
[code-only review] Already merged, but still requested by @MiquelAdell. In-line comments:
| onSave: (periodDate: PeriodDate) => void; | ||
| }; | ||
|
|
||
| function useGetYears(props: { period: PeriodDate }) { |
There was a problem hiding this comment.
Typically, we put props+component+hooks/helper functions
| })} | ||
| </PeriodsContainer> | ||
| )} | ||
| </ConfirmationDialog> |
There was a problem hiding this comment.
We may add a blank line between non-single-line components at the same indentation level (for example 129/130)
| label={i18n.t("End date of data input")} | ||
| minDate={periodDate.startDate} | ||
| className="datepicker" | ||
| format="yyyy-MM-DD" |
There was a problem hiding this comment.
repeated prop format. Either a variable or a wrapper component that adds common props.
| const onUpdatePeriodDate = (date: string, fieldName: "startDate" | "endDate") => { | ||
| const newValues = periodDate?.setDates(date, fieldName); | ||
| setPeriodDate(newValues); | ||
| }; |
| console.debug(JSON.stringify(response, null, 4)); | ||
| }, | ||
| error => { | ||
| console.error(error); |
There was a problem hiding this comment.
[good practices] process.exit(1) on the error branch
| } | ||
|
|
||
| setDates(date: string, fieldName: "startDate" | "endDate"): PeriodDate { | ||
| return this._update({ [fieldName]: date }); |
There was a problem hiding this comment.
That's unfortunate, but TS does not enforce type-safety when building objects from dynamic keys:
A 100% type-safe workaround is using: const newAttrs = fromPairs([[fieldName, date]]) (fromPairs from ts-utils or EST lodash should both work)
| @@ -54,6 +53,31 @@ export function convertToCategories(d2Categories: D2Category[]): Category[] { | |||
| })); | |||
| } | |||
|
|
|||
| export function convertAttributeValueToDate( | |||
There was a problem hiding this comment.
this is a very specific non-generic function, utils.ts probably is not be the best place.
| periodDate: PeriodDate; | ||
| outcomeDate: PeriodDate; | ||
| outputDate: PeriodDate; | ||
| }; |
There was a problem hiding this comment.
[no need to change]
I see you created an entity+repo for DataSetPeriodDate.
This is something we have been discussing lately, and I'd like to confirm with @xurxodev, but from our last chat about this, he stated that having different "flavors" or entities is not correct. I see two alternatives:
- Add these 3 fields would be in DataSet. I think this is the most simple option. And then we simply improve the existing DataSet/DataSetRepository. You may then say that we typically don't need them and getting the info from the attribute is very costly (i don't think it's the case here). Then we'd see alternatives.
-Have a DataSetPeriodDate, but not as flavor, but as its own entity. So having dataSetId: Id + 3 fields. Typically we do this if the entity has sense in isolation. Here it's 1:1 relation, it does not seem a typical case for this.
📌 References
📝 Implementation
📹 Screenshots/Screen capture
Project-Configuration-app-period_dates.webm
🔥 Notes to the tester
#8695uy4nz