-
Notifications
You must be signed in to change notification settings - Fork 2
refactor: standardize date objects across project #472
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,186 @@ | ||
| # Date Standardization — Session Notes | ||
|
|
||
| **Branch:** `refactor/standardize-date-objects` | ||
| **Session:** 2026-04-27 — 2026-05-02 | ||
| **Commit:** `b0a3a7b` — _refactor: standardize date objects across project_ | ||
| **PR URL:** https://github.com/appKom/opptak/pull/new/refactor/standardize-date-objects | ||
|
|
||
| > This file is a working memo of what happened in the session. It is **not** committed to git (kept locally only). | ||
|
|
||
| --- | ||
|
|
||
| ## 1. Context at session start | ||
|
|
||
| The branch had a large in-progress refactor: period / applicant / committee date fields changed from `string` to `Date` end-to-end. Already in the working tree (uncommitted) before the session: | ||
|
|
||
| - `lib/types/types.ts` — types flipped from `string` → `Date`. | ||
| - `lib/utils/parseDates.ts` (new) — central client-side parsing helpers (`parsePeriodDates`, `parseApplicantDates`, `parseCommitteeDates`). | ||
| - `lib/api/{applicantApi,committeesApi,periodApi}.ts` — fetchers run `parseDates` on JSON responses. | ||
| - `pages/api/...` — POST/PUT handlers run `parseDates` on `req.body`. | ||
| - `lib/utils/{validators,validateApplication,convertIsoToScheduleFormat,dateUtils}.ts` — adapted to handle Dates. | ||
| - `pages/{apply,admin,committee,committees}` — direct Date comparisons replacing string-vs-Date hacks. | ||
| - `lib/mongo/{periods,applicants}.ts` — mongo queries now pass Date objects. | ||
| - `scripts/migrate-dates.js` — in-place migration script (string → BSON Date) for any DB the URI points to. | ||
|
|
||
| --- | ||
|
|
||
| ## 2. Pre-test analysis — bugs identified by reading the diff | ||
|
|
||
| ### Bug 1 — `validateCommittee` rejected parsed `Date` objects | ||
| `lib/utils/validators.ts` (in `validateCommittee`): | ||
|
|
||
| ```ts | ||
| data.availabletimes.every( | ||
| (time: { start: string; end: string }) => | ||
| typeof time.start === "string" && typeof time.end === "string", | ||
| ); | ||
| ``` | ||
|
|
||
| Called *after* `parseCommitteeDates(req.body)` ran, so `start`/`end` were already `Date` objects → `typeof === "string"` was always `false` → 400 "Invalid data format" on every committee submission. | ||
|
|
||
| **Fix:** check for `Date` instances instead. | ||
|
|
||
| ### Bug 2 — `isApplicantType(req.body, ...)` saw `date: undefined` | ||
| `pages/api/applicants/index.ts`: | ||
|
|
||
| ```ts | ||
| const requestBody = parseApplicantDates(req.body); // new object, not mutation | ||
| requestBody.date = new Date(); | ||
| ... | ||
| if (!isApplicantType(req.body, period)) { // ← raw req.body, no date Date | ||
| ``` | ||
|
|
||
| Pre-refactor `requestBody = req.body` was an alias, so `requestBody.date = new Date()` mutated `req.body.date`. Post-refactor `parseApplicantDates` returns a fresh object via spread, so `req.body.date` stayed `undefined`. Validator's `date instanceof Date` check failed → 400. | ||
|
|
||
| **Fix:** call `isApplicantType(requestBody, period)`. | ||
|
|
||
| ### Two pre-existing concerns logged, not regressions of this PR: | ||
| - Two `periods` rows with corrupted year data (`Fredrik Hansteen` year 1, `Testeeeeeee` year 567). | ||
| - React warning `<ul>` inside `<p>` in `SendOutInterviews.tsx`. | ||
| - Image aspect-ratio warning on `Online_bla.svg`. | ||
|
|
||
| --- | ||
|
|
||
| ## 3. Bugs found later during testing | ||
|
|
||
| ### Bug 3 — `formatDateNorwegian` rendered double periods (`09.. mar`) | ||
| `lib/utils/dateUtils.ts` — Norwegian locale's `toLocaleDateString({ day: "2-digit" })` returns `"09."` (with trailing period). Template `${day}. ${month}` then added another period → `"09.. mar"`. | ||
|
|
||
| **Fix:** strip trailing period from `day`, mirroring what was already done for `month`: | ||
| ```ts | ||
| const day = date.toLocaleDateString("nb-NO", { day: "2-digit", timeZone: "Europe/Oslo" }) | ||
| .replace(".", ""); | ||
| ``` | ||
|
|
||
| ### Bug 4 — `Schedule.tsx getDatesWithinPeriod` lost the `+2h` shift | ||
| The original had: | ||
| ```ts | ||
| startDate.setHours(startDate.getHours() + 2); | ||
| ``` | ||
| This was a workaround so periods stored as midnight-Oslo (`2040-07-15T22:00:00Z` = `2040-07-16 00:00 Oslo`) rendered correctly in the schedule grid. The refactor dropped the shift, causing the schedule to render days *before* the actual period start. Slots clicked on those days fell **outside** the validator's interview window → 400. | ||
|
|
||
| **Fix:** reinstate the shift on local copies (without mutating the input Date): | ||
| ```ts | ||
| const startDate = new Date( | ||
| (periodTime.start instanceof Date ? periodTime.start : new Date(periodTime.start)).getTime(), | ||
| ); | ||
| startDate.setHours(startDate.getHours() + 2); | ||
| // same for endDate | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 4. Migration strategy chosen | ||
|
|
||
| **Decision:** rather than running `migrate-dates.js` in place against `development`, build a parallel migrated DB via aggregation `$out`. Keeps `development` untouched while we smoke-test. | ||
|
|
||
| **Steps actually executed:** | ||
|
|
||
| 1. Installed `mongodb-database-tools` via Homebrew tap `mongodb/brew`. | ||
| 2. Took a backup with `mongodump`: | ||
| ``` | ||
| ~/mongodb-backups/opptaksside-20260427-175203/ | ||
| ``` | ||
| (553 docs across 5 collections, ~1.1 MB) | ||
| 3. Wrote `scripts/migrate-to-new-db.js` — Node + `mongodb` driver, runs aggregation pipelines that read from `development` and `$out` to a new DB called `Date_dev`. | ||
| 4. Ran `node scripts/migrate-to-new-db.js` — copied: | ||
| - `periods` (50 docs) — `applicationPeriod.{start,end}` and `interviewPeriod.{start,end}` converted via `$toDate`. | ||
| - `applications` (184 docs) — `selectedTimes[].{start,end}` via `$map` + `$toDate`. | ||
| - `committees` (65 docs) — `availabletimes[].{start,end,room}` via `$map`. | ||
| - `interviews` (243 docs) — straight copy. | ||
| - `rooms` (11 docs) — straight copy. | ||
| 5. Updated `.env.local` `MONGODB_URI` to point at `/Date_dev`. Restarted dev server. | ||
|
|
||
| **Strict rule established mid-session (saved to memory):** never write to the `development` MongoDB without **two ⚠️⚠️ confirmations** from the user. Read-only ops fine. | ||
|
|
||
| --- | ||
|
|
||
| ## 5. Smoke test results | ||
|
|
||
| Executed step-by-step against `Date_dev`: | ||
|
|
||
| | Step | What was tested | Result | | ||
| |---|---|---| | ||
| | 1 | App connects to `Date_dev`, `/apply` cards render, dates formatted as `09. mar` | ✅ (after Bug 3 fix) | | ||
| | 2 | `/admin` table renders all 50 periods with `DD.MM.YYYY` | ✅ | | ||
| | 3 | POST `/api/applicants` → `201`, doc has BSON Date for `selectedTimes[].{start,end}` and `date` | ✅ (after Bug 2 + Bug 4 fixes) | | ||
| | 4 | POST `/api/committees/times/[periodId]` → `201`, doc has BSON Date for `availabletimes[].{start,end}` | ✅ (after Bug 1 fix) | | ||
|
|
||
| Compass verification at the end confirmed both new docs in `Date_dev.applications` and `Date_dev.committees` had unquoted `Date(...)` values for the date fields (vs. quoted strings for `name`, `committee`, `room`, etc.). | ||
|
|
||
| --- | ||
|
|
||
| ## 6. Untested paths (acknowledged gaps) | ||
|
|
||
| **High-value:** | ||
| - POST `/api/periods` — creating a new period. Exercises `DatePickerInput` + `isPeriodType` + `parsePeriodDates`. | ||
| - PUT `/api/applicants` — editing an existing application. Same fix applied as POST but only POST was runtime-verified. | ||
| - Round-trip read-back of `selectedTimes` — does **Endre søknad** correctly pre-select previous slots in the schedule? | ||
|
|
||
| **Important:** | ||
| - `formatDateHours` in email/SMS templates (gated by `NODE_ENV=production`, deliberately skipped per testing rule — but means production interview-email rendering is unverified). | ||
| - Hidden-applicant placeholder — `lib/mongo/applicants.ts:214` change from `selectedTimes = [{start:"Skjult", end:"Skjult"}]` to `[{start:new Date(0), end:new Date(0)}]` only fires when application period is open or 5+ days after interview end. Will display `01.01.1970` to committees in that window. | ||
|
|
||
| **Lower priority:** | ||
| - DST transition periods — `+2h` shift assumes constant Oslo offset; periods spanning the last Sunday of March/October may render off-by-one for one day. | ||
| - `getCurrentPeriods` in `lib/mongo/periods.ts` — not clearly wired into any active route; worth a `grep` to confirm dead code or active. | ||
| - `/committees` public listing, `Søkere` tab in committee view — read paths, lowest risk but unverified. | ||
|
|
||
| --- | ||
|
|
||
| ## 7. Final commit | ||
|
|
||
| ``` | ||
| b0a3a7b refactor: standardize date objects across project | ||
| 28 files changed, 683 insertions(+), 205 deletions(-) | ||
| + lib/utils/parseDates.ts | ||
| + scripts/migrate-dates.js | ||
| + scripts/migrate-to-new-db.js | ||
| + 25 modified | ||
| ``` | ||
|
|
||
| Pushed to `origin/refactor/standardize-date-objects`. PR not opened yet — open via the URL above. | ||
|
|
||
| `.env.local` still points at `Date_dev` locally. To return to normal dev work against `development`, swap the path segment back. (`.env.local` is gitignored.) | ||
|
|
||
| --- | ||
|
|
||
| ## 8. Outstanding decisions for production | ||
|
|
||
| Not started in this session — needs its own conversation: | ||
|
|
||
| 1. **Backup production** (`mongodump` against the prod URI) — required. | ||
| 2. **Choose migration pattern for prod:** | ||
| - `migrate-dates.js` — in-place, simpler, destructive (irreversible without restore). | ||
| - `migrate-to-new-db.js` — non-destructive, but requires a URI swap on prod environment + cleanup of the source DB later. | ||
| 3. **Run prod migration** — needs ⚠️⚠️ double-confirm. | ||
| 4. **Deploy** the merged branch. | ||
| 5. **Optional cleanup**: drop `Date_dev`; clean up the two corrupted-year periods. | ||
|
|
||
| --- | ||
|
|
||
| ## 9. Misc observations from the session | ||
|
|
||
| - Repo URL has moved server-side from `appKom/opptaksside` to `appKom/opptak`. Push still works via redirect; future cleanup: `git remote set-url origin https://github.com/appKom/opptak.git`. | ||
| - 50 dependabot vulnerabilities exist on the default branch (unrelated to this PR). | ||
| - `.idea/` is currently untracked (not in `.gitignore`). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,9 +18,9 @@ interface TimeSlot { | |
| available: boolean; | ||
| } | ||
|
|
||
| interface IsoTimeSlot { | ||
| start: string; | ||
| end: string; | ||
| interface DateTimeSlot { | ||
| start: Date; | ||
| end: Date; | ||
| } | ||
|
|
||
| export default function Schedule({ | ||
|
|
@@ -37,14 +37,24 @@ export default function Schedule({ | |
| periodTime: any, | ||
| ): { [date: string]: string } => { | ||
| if (!periodTime) return {}; | ||
| const startDate = new Date(periodTime.start); | ||
| const startDate = new Date( | ||
| (periodTime.start instanceof Date | ||
| ? periodTime.start | ||
| : new Date(periodTime.start) | ||
| ).getTime(), | ||
| ); | ||
| startDate.setHours(startDate.getHours() + 2); | ||
| const endDate = new Date(periodTime.end); | ||
| const endDate = new Date( | ||
| (periodTime.end instanceof Date | ||
| ? periodTime.end | ||
| : new Date(periodTime.end) | ||
| ).getTime(), | ||
| ); | ||
| endDate.setHours(endDate.getHours() + 2); | ||
| const dates: { [date: string]: string } = {}; | ||
| const dayNames = ["Søn", "Man", "Tir", "Ons", "Tor", "Fre", "Lør"]; | ||
|
|
||
| let currentDate = new Date(startDate); | ||
| let currentDate = new Date(startDate.getTime()); | ||
|
|
||
| while (currentDate <= endDate) { | ||
| const dayIndex = currentDate.getDay(); | ||
|
|
@@ -72,8 +82,8 @@ export default function Schedule({ | |
| return [hour, minute]; | ||
| }, []); | ||
|
|
||
| const convertToIso = useCallback( | ||
| (date: string, timeSlot: string): IsoTimeSlot => { | ||
| const convertToDateSlot = useCallback( | ||
| (date: string, timeSlot: string): DateTimeSlot => { | ||
| const [startTimeStr, endTimeStr] = timeSlot.split(" - "); | ||
| const [year, month, day] = date.split("-").map(Number); | ||
|
|
||
|
|
@@ -88,8 +98,8 @@ export default function Schedule({ | |
| ); | ||
|
|
||
| return { | ||
| start: startTime.toISOString(), | ||
| end: endTime.toISOString(), | ||
| start: startTime, | ||
| end: endTime, | ||
| }; | ||
| }, | ||
| [parseTime], | ||
|
|
@@ -118,21 +128,21 @@ export default function Schedule({ | |
| }); | ||
| }); | ||
|
|
||
| const isoTimeSlotsForExport = allAvailableTimes.map((slot) => | ||
| convertToIso(slot.date, slot.time), | ||
| const dateTimeSlotsForExport = allAvailableTimes.map((slot) => | ||
| convertToDateSlot(slot.date, slot.time), | ||
| ); | ||
|
|
||
| setApplicationData((prevData: any) => ({ | ||
| ...prevData, | ||
| selectedTimes: isoTimeSlotsForExport, | ||
| selectedTimes: dateTimeSlotsForExport, | ||
| })); | ||
| setIsInitialized(true); | ||
| }, [ | ||
| isInitialized, | ||
| applicationData.selectedTimes, | ||
| periodTime, | ||
| timeSlots, | ||
| convertToIso, | ||
| convertToDateSlot, | ||
| setApplicationData, | ||
| ]); | ||
|
|
||
|
|
@@ -185,26 +195,30 @@ export default function Schedule({ | |
| }); | ||
| }); | ||
|
|
||
| const isoTimeSlotsForExport = dataToSend.map((slot) => | ||
| convertToIso(slot.date, slot.time), | ||
| const dateTimeSlotsForExport = dataToSend.map((slot) => | ||
| convertToDateSlot(slot.date, slot.time), | ||
| ); | ||
|
|
||
| // Queue the update instead of calling setApplicationData directly | ||
| setPendingUpdate(isoTimeSlotsForExport); | ||
| setPendingUpdate(dateTimeSlotsForExport); | ||
|
|
||
| return newCells; | ||
| }); | ||
| }; | ||
|
|
||
| const dates = getDatesWithinPeriod(periodTime); | ||
|
|
||
| let weekDates: { [date: string]: IsoTimeSlot[] } = {}; | ||
| let weekDates: { [date: string]: DateTimeSlot[] } = {}; | ||
|
|
||
| Object.keys(dates).forEach((date) => { | ||
| weekDates[date] = []; | ||
| applicationData.selectedTimes?.forEach((slot) => { | ||
| if (slot?.start?.includes(date) && slot?.end) { | ||
| weekDates[date].push({ start: slot.start, end: slot.end }); | ||
| if (slot?.start && slot?.end) { | ||
| const start = new Date(slot.start as unknown as string | number | Date); | ||
| const end = new Date(slot.end as unknown as string | number | Date); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tilsvarende her |
||
| if (start.toISOString().includes(date)) { | ||
| weekDates[date].push({ start, end }); | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
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.
Hvorfor trengs denne sjekken her? Vil jo helst at den alltid skal være
instanceof Date