Add default cultivation dates: schema fields, core util, app routes#293
Conversation
…ne default dates for cultivations
…ue date are used for b_lu_start and b_lu_end if available when adding a field with a cultivation
🦋 Changeset detectedLatest commit: d30a616 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds MM-DD default start/harvest fields to the cultivations catalogue schema and data, introduces CultivationDefaultDates and getDefaultDatesOfCultivation in fdm-core (exported), updates app routes to use the computed defaults when creating fields/cultivations, and adds/updates tests and changesets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant App as fdm-app Route
participant Core as fdm-core
participant DB as DB / catalogue
U->>App: create field/cultivation (no dates)
App->>Core: getDefaultDatesOfCultivation(principal, farm, catalogue, year)
Core->>DB: read enabled catalogue & cultivation defaults
DB-->>Core: {b_lu_start_default, b_date_harvest_default, harvestable}
Core-->>App: {b_lu_start, b_lu_end?}
note right of App: set b_start = Jan 1 of year
App->>DB: addField(..., b_start)
App->>DB: addCultivation(..., b_lu_start, b_lu_end?)
DB-->>App: Created
App-->>U: Redirect / Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #293 +/- ##
===============================================
+ Coverage 93.07% 93.11% +0.03%
===============================================
Files 81 81
Lines 13125 13256 +131
Branches 1327 1347 +20
===============================================
+ Hits 12216 12343 +127
- Misses 907 911 +4
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
changed the title
coderabbitai
Bot
added
branch:development
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
fdm-core/src/db/schema.ts (1)
374-375: Consider adding format validation for date fields.The new columns are correctly added to store default dates in MM-dd format. However, consider adding a CHECK constraint to validate the format at the database level to prevent invalid data insertion.
For example, you could add regex validation like:
b_lu_start_default: text().check(sql`b_lu_start_default IS NULL OR b_lu_start_default ~ '^(0[1-9]|1[0-2])-(0[1-9]|[12][0-9]|3[01])$'`), b_date_harvest_default: text().check(sql`b_date_harvest_default IS NULL OR b_date_harvest_default ~ '^(0[1-9]|1[0-2])-(0[1-9]|[12][0-9]|3[01])$'`),This would ensure only valid MM-dd formatted strings are stored.
fdm-core/src/cultivation.test.ts (2)
803-805: Remove stray inline comment"// This is the problematic line" looks leftover/noise in a passing test. Please remove to keep tests clean.
- b_lu_croprotation: "cereal", // This is the problematic line + b_lu_croprotation: "cereal",
932-1034: Add edge-case tests for defaults helperCoverage is good. Please add:
- harvestable = 'none' → only start date is returned (no end).
- missing b_lu_start_default → helper throws a clear error.
I can draft the test cases if you want.
fdm-core/src/cultivation.ts (2)
145-162: Format-only regex is insufficient for real datesRegex validates MM-dd shape but not actual month/day validity (e.g., 02-30). Consider either:
- validating via Date parsing and rejecting invalid dates, or
- centralizing a shared validator to keep schema/data checks consistent across helpers.
Would you like me to propose a small validator and reuse it here and in the defaults helper?
266-268: Typo in comment"occured" → "occurred".
- // occured in the previous year, so we use the previous year for the start date. + // occurred in the previous year, so we use the previous year for the start date.fdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsx (1)
419-491: Consider limiting concurrency to protect external APIs and DBCreating many fields/cultivations in parallel can overload DB/NMI. Use a small concurrency pool (e.g., p-limit 3–5) instead of Promise.all over all items.
fdm-data/src/cultivations/hash.test.ts (1)
32-32: Avoid brittle hard-coded golden hashHash can change if field ordering/normalization changes. Consider snapshotting the sorted payload or asserting structural properties instead of a fixed hex.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
fdm-core/src/db/migrations/0014_clean_calypso.sqlis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/meta/0014_snapshot.jsonis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/meta/_journal.jsonis excluded by!fdm-core/src/db/migrations/**
📒 Files selected for processing (17)
.changeset/big-cars-like.md(1 hunks).changeset/cold-jars-marry.md(1 hunks).changeset/evil-walls-read.md(1 hunks).changeset/lovely-mammals-sneeze.md(1 hunks).changeset/proud-banks-train.md(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx(2 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsx(2 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx(2 hunks)fdm-core/src/cultivation.d.ts(1 hunks)fdm-core/src/cultivation.test.ts(15 hunks)fdm-core/src/cultivation.ts(4 hunks)fdm-core/src/db/schema.ts(1 hunks)fdm-core/src/fertilizer.test.ts(1 hunks)fdm-core/src/index.ts(1 hunks)fdm-data/src/cultivations/catalogues/brp.ts(1 hunks)fdm-data/src/cultivations/d.ts(1 hunks)fdm-data/src/cultivations/hash.test.ts(10 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-01-24T11:39:57.805Z
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-core/src/cultivation.ts:625-671
Timestamp: 2025-01-24T11:39:57.805Z
Learning: Every cultivation has an associated `cultivationTerminating` record created by `addCultivation`, with `b_terminate_date` initially set to null. Therefore, `updateCultivation` can safely assume the record exists and only needs to handle updates.
Applied to files:
fdm-core/src/cultivation.test.ts
📚 Learning: 2025-01-23T15:18:57.212Z
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-core/src/db/schema.ts:407-426
Timestamp: 2025-01-23T15:18:57.212Z
Learning: In the farm data model, each cultivation (b_lu) can have only one termination date but can have multiple harvest dates. This is enforced through the database schema where cultivationTerminating uses b_lu as primary key while cultivationHarvesting uses a composite primary key of b_id_harvestable and b_lu.
Applied to files:
fdm-core/src/cultivation.test.ts
📚 Learning: 2025-08-11T12:24:32.200Z
Learnt from: SvenVw
PR: SvenVw/fdm#233
File: fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx:53-53
Timestamp: 2025-08-11T12:24:32.200Z
Learning: In `fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx`, the NMI API for cultivations guarantees that each year will be unique in the cultivation history data, so using `cultivation.year` as a React list key is safe and won't cause duplicate key warnings.
Applied to files:
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsx
🧬 Code graph analysis (6)
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx (1)
fdm-core/src/cultivation.ts (2)
getDefaultDatesOfCultivation(191-289)addCultivation(308-492)
fdm-data/src/cultivations/hash.test.ts (1)
fdm-data/src/cultivations/hash.ts (1)
hashCultivation(4-28)
fdm-core/src/cultivation.ts (4)
fdm-core/src/db/schema.ts (3)
cultivationsCatalogueTypeInsert(385-386)cultivationsCatalogueTypeSelect(383-384)cultivationsCatalogue(356-381)fdm-core/src/index.ts (3)
getDefaultDatesOfCultivation(40-40)FdmType(72-72)PrincipalId(25-25)fdm-core/src/cultivation.d.ts (1)
CultivationDefaultDates(62-65)fdm-core/src/authorization.ts (1)
checkPermission(156-240)
fdm-core/src/cultivation.test.ts (2)
fdm-core/src/cultivation.ts (2)
addCultivationToCatalogue(105-172)getDefaultDatesOfCultivation(191-289)fdm-core/src/index.ts (2)
addCultivationToCatalogue(39-39)getDefaultDatesOfCultivation(40-40)
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsx (3)
fdm-core/src/cultivation.ts (1)
getDefaultDatesOfCultivation(191-289)fdm-core/src/index.ts (1)
getDefaultDatesOfCultivation(40-40)fdm-app/app/lib/fdm-migrate.server.js (1)
fdm(52-56)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (2)
fdm-core/src/cultivation.ts (1)
getDefaultDatesOfCultivation(191-289)fdm-core/src/index.ts (1)
getDefaultDatesOfCultivation(40-40)
🔇 Additional comments (6)
.changeset/big-cars-like.md (1)
1-5: LGTM!The changeset correctly documents the minor version bump for
@svenvw/fdm-dataand describes the addition of the new default date fields to the BRP catalogue.fdm-core/src/fertilizer.test.ts (1)
995-1002: Good refactor: forEach is more appropriate here.Changing from
maptoforEachis the right choice since the iteration is only for side-effects (assertions) and the returned array was not used..changeset/lovely-mammals-sneeze.md (1)
1-5: LGTM!The changeset correctly documents the minor version bump for
@svenvw/fdm-coreand describes the new exported helper functiongetDefaultDatesOfCultivation.fdm-data/src/cultivations/d.ts (1)
31-32: LGTM!The new fields are correctly added to the
CatalogueCultivationIteminterface with appropriate types (string | null) to represent the MM-dd formatted default dates.fdm-core/src/index.ts (1)
40-40: LGTM! Verify the implementation.The new export correctly exposes
getDefaultDatesOfCultivationas part of the public API. The function is properly positioned in alphabetical order within the cultivation exports.Since the implementation of
getDefaultDatesOfCultivationis not included in the files under review, ensure that:
- The function is properly implemented in
fdm-core/src/cultivation.ts- It handles all edge cases (invalid dates, null values, harvestable types)
- It follows the MM-dd → full Date conversion logic as per the PR objectives
- Comprehensive tests exist in
fdm-core/src/cultivation.test.tsfdm-data/src/cultivations/hash.test.ts (1)
23-33: Tests updated correctly for new fieldsIncluding b_lu_start_default and b_date_harvest_default and updating the golden hash looks good.
| const currentYear = Number(calendar) | ||
|
|
||
| const cultivationDefaultDates = await getDefaultDatesOfCultivation( | ||
| fdm, | ||
| session.principal_id, | ||
| b_id_farm, | ||
| b_lu_catalogue, | ||
| currentYear, | ||
| ) | ||
| const b_start = new Date(`${currentYear}-01-01`) | ||
| const b_lu_start = cultivationDefaultDates.b_lu_start | ||
| const b_lu_end = cultivationDefaultDates.b_lu_end | ||
| const b_end = undefined |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add guards for calendar year and invalid default dates
- Validate Number(calendar) yields an integer.
- Ensure b_lu_start/b_lu_end are valid Dates before calling addCultivation.
Apply this diff:
- const currentYear = Number(calendar)
+ const currentYear = Number(calendar)
+ if (!Number.isInteger(currentYear)) {
+ throw new Error(`invalid calendar year: ${calendar}`)
+ }
const cultivationDefaultDates = await getDefaultDatesOfCultivation(
fdm,
session.principal_id,
b_id_farm,
b_lu_catalogue,
currentYear,
)
const b_start = new Date(`${currentYear}-01-01`)
- const b_lu_start = cultivationDefaultDates.b_lu_start
- const b_lu_end = cultivationDefaultDates.b_lu_end
+ const b_lu_start = cultivationDefaultDates.b_lu_start
+ const b_lu_end = cultivationDefaultDates.b_lu_end
+ if (Number.isNaN(b_lu_start.getTime())) {
+ throw new Error(`Invalid default start date for cultivation ${b_lu_catalogue}`)
+ }
+ if (b_lu_end && Number.isNaN(b_lu_end.getTime())) {
+ throw new Error(`Invalid default end date for cultivation ${b_lu_catalogue}`)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const currentYear = Number(calendar) | |
| const cultivationDefaultDates = await getDefaultDatesOfCultivation( | |
| fdm, | |
| session.principal_id, | |
| b_id_farm, | |
| b_lu_catalogue, | |
| currentYear, | |
| ) | |
| const b_start = new Date(`${currentYear}-01-01`) | |
| const b_lu_start = cultivationDefaultDates.b_lu_start | |
| const b_lu_end = cultivationDefaultDates.b_lu_end | |
| const b_end = undefined | |
| const currentYear = Number(calendar) | |
| if (!Number.isInteger(currentYear)) { | |
| throw new Error(`invalid calendar year: ${calendar}`) | |
| } | |
| const cultivationDefaultDates = await getDefaultDatesOfCultivation( | |
| fdm, | |
| session.principal_id, | |
| b_id_farm, | |
| b_lu_catalogue, | |
| currentYear, | |
| ) | |
| const b_start = new Date(`${currentYear}-01-01`) | |
| const b_lu_start = cultivationDefaultDates.b_lu_start | |
| const b_lu_end = cultivationDefaultDates.b_lu_end | |
| if (Number.isNaN(b_lu_start.getTime())) { | |
| throw new Error(`Invalid default start date for cultivation ${b_lu_catalogue}`) | |
| } | |
| if (b_lu_end && Number.isNaN(b_lu_end.getTime())) { | |
| throw new Error(`Invalid default end date for cultivation ${b_lu_catalogue}`) | |
| } | |
| const b_end = undefined |
There was a problem hiding this comment.
Why? The function getCalendar should return a calendar year and getDefaultDatesOfCultivation will return date objects
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the getCalendar function implementation
echo "=== Searching for getCalendar function ==="
ast-grep --pattern 'export function getCalendar($$$) { $$$ }'
echo -e "\n=== Searching for getCalendar with alternate patterns ==="
rg -nP --type=ts -C5 'function getCalendar'
echo -e "\n=== Finding getCalendar in imports ==="
rg -nP --type=ts 'import.*getCalendar'
Length of output: 5433
🏁 Script executed:
#!/bin/bash
# Examine the getDefaultDatesOfCultivation function implementation
echo "=== Examining getDefaultDatesOfCultivation implementation ==="
ast-grep --pattern 'export async function getDefaultDatesOfCultivation($$$) { $$$ }'
echo -e "\n=== Alternative search for getDefaultDatesOfCultivation ==="
rg -nP --type=ts -A30 'async function getDefaultDatesOfCultivation'
Length of output: 2258
🏁 Script executed:
#!/bin/bash
# Get the complete implementation of getDefaultDatesOfCultivation
echo "=== Full getDefaultDatesOfCultivation implementation ==="
rg -nP --type=ts -A100 'export async function getDefaultDatesOfCultivation' fdm-core/src/cultivation.ts | head -120
Length of output: 4152
🏁 Script executed:
#!/bin/bash
# Check the CultivationDefaultDates type definition
echo "=== CultivationDefaultDates type definition ==="
rg -nP --type=ts -C3 'type CultivationDefaultDates'
Length of output: 447
@SvenVw, I understand your point. Let me clarify what I found by examining the actual implementations:
getCalendar currently just returns the route parameter as-is:
export function getCalendar(params: Params): string {
const calendar = params.calendar as string
return calendar
}It performs no validation that the value is actually a valid year. So Number(calendar) could theoretically produce NaN or a non-integer if the route param is malformed.
getDefaultDatesOfCultivation does construct Date objects:
cultivationDefaultDates.b_lu_start = new Date(
`${year}-${cultivationsCatalogue[0].b_lu_start_default}`
)However, it doesn't validate that the resulting Date is valid. JavaScript's new Date() with invalid input (like "2025-13-32") creates an Invalid Date object where getTime() returns NaN.
So while your assumptions about what these functions should do are reasonable, the current implementations don't actually enforce those guarantees. The validation catches:
- Non-numeric or malformed calendar params
- Invalid Date objects from malformed catalogue data
That said, if you're confident the route params and catalogue data are always well-formed in practice, you can skip these checks. The decision is yours!
🧬 Code Graph Analysis Results
fdm-app/app/lib/calendar.ts
-
Snippet: getCalendar
- File: fdm-app/app/lib/calendar.ts
- Range: 7-11
- Content:
export function getCalendar(params: Params): string { const calendar = params.calendar as string return calendar } - Relevance: Shows that getCalendar simply returns the calendar value from route params (string). This supports evaluating whether it should return a year number instead of a string.
-
Snippet: getTimeframe
- File: fdm-app/app/lib/calendar.ts
- Range: 13-37
- Content:
export function getTimeframe(params: Params): Timeframe { const calendar = getCalendar(params) const timeframe = { start: new Date(`${yearStart}-01-01T00:00:00.000Z`), end: new Date(`${yearEnd}-12-31T23:59:59.999Z`), } // Check if calendar is year and create a timeframe if (calendar) { // Try to coerce to year const year = Number(calendar) if (!Number.isNaN(year)) { // Check if year is supported if (year < yearStart || year > yearEnd) { throw new Error(`Unsupported year: ${calendar}`) } // Set start and end date timeframe.start = new Date(`${year}-01-01T00:00:00.000Z`) timeframe.end = new Date(`${year}-12-31T23:59:59.999Z`) } } return timeframe } - Relevance: Indicates that only if calendar coerces to a numeric year is the timeframe adjusted to that year. This is central to the user’s expectation that the calendar parameter represents a year.
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx
-
Snippet: usage of calendar value as current year
- File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx
- Range: 0-0 (context shown in action)
- Content (relevant excerpt):
const currentYear = Number(calendar) - Relevance: Demonstrates how the code currently treats the calendar value as a year number when deriving default cultivation dates. Supports the idea that getCalendar should yield a value representing a year.
-
Snippet: getDefaultDatesOfCultivation usage
- File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx
- Range: (see surrounding code where cultivationDefaultDates is computed)
- Content (summarized description):
- Calling getDefaultDatesOfCultivation with parameters:
- fdm, session.principal_id, b_id_farm, b_lu_catalogue, currentYear
- Then using cultivationDefaultDates.b_lu_start and cultivationDefaultDates.b_lu_end
- Calling getDefaultDatesOfCultivation with parameters:
- Relevance: Illustrates that the default dates function returns date objects used to initialize cultivation spans for a new field, aligning with the user’s expectation that the function returns actual Date objects.
fdm-core/src/cultivation.ts
- Doc: getDefaultDatesOfCultivation
- File: fdm-core/src/cultivation.ts
- Range: 191-289
- Description:
- Function signature:
- getDefaultDatesOfCultivation(fdm, principal_id, b_id_farm, b_lu_catalogue, year) -> Promise
- Returns:
- CultivationDefaultDates with:
- b_lu_start: Date
- b_lu_end: Date | undefined
- computed using enabled catalogues and catalogue defaults (b_lu_start_default, b_date_harvest_default)
- CultivationDefaultDates with:
- Behavior highlights:
- Validates permissions
- Retrieves enabled cultivation catalogues for farm
- Looks up catalogue entry for b_lu_catalogue
- Constructs default start date from year and b_lu_start_default
- If harvestable type is "once" and a default harvest date exists, sets b_lu_end accordingly
- If end date is before start date, adjusts start date to previous year
- Relevant error handling:
- Throws errors if catalogue entry not found, or on permission/DB issues
- Function signature:
- Relevance: Directly addresses the user concern that getDefaultDatesOfCultivation should return Date objects representing default cultivation dates. This description clarifies expected return type and behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.changeset/cold-jars-marry.md (1)
5-5: Clarify thatb_date_harvest_defaultis the harvest date, not the end date.The description states "default start and end dates" but
b_date_harvest_defaultstores the harvest date. The end date (b_lu_end) is derived from the harvest date for single-harvest cultivations. Consider rephrasing for accuracy.Apply this diff to clarify:
-Adds `b_lu_start_default` and `b_date_harvest_default` to cultivations catalogue as the default start and end dates of a cultivation +Adds `b_lu_start_default` and `b_date_harvest_default` to cultivations catalogue as the default start and harvest dates of a cultivationfdm-core/src/cultivation.ts (2)
145-162: Enhance date validation to catch semantically invalid dates.The regex validates MM-dd format but allows impossible dates like "02-31" (February 31st) or "04-31" (April 31st). These will pass validation here but create Invalid Date objects when parsed in
getDefaultDatesOfCultivation.Consider validating semantic correctness by attempting to parse the date:
// Validate if b_lu_start_default and b_date_harvest_default follows format MM-dd const dateRegex = /^(0[1-9]|1[0-2])-(0[1-9]|[1-2][0-9]|3[0-1])$/ if ( properties.b_lu_start_default && !dateRegex.test(properties.b_lu_start_default) ) { throw new Error( "Invalid b_lu_start_default format. Expected MM-dd.", ) } +if (properties.b_lu_start_default) { + const testDate = new Date(`2024-${properties.b_lu_start_default}`) + if (Number.isNaN(testDate.getTime())) { + throw new Error( + `Invalid b_lu_start_default date: ${properties.b_lu_start_default} is not a valid calendar date.`, + ) + } +} if ( properties.b_date_harvest_default && !dateRegex.test(properties.b_date_harvest_default) ) { throw new Error( "Invalid b_date_harvest_default format. Expected MM-dd.", ) } +if (properties.b_date_harvest_default) { + const testDate = new Date(`2024-${properties.b_date_harvest_default}`) + if (Number.isNaN(testDate.getTime())) { + throw new Error( + `Invalid b_date_harvest_default date: ${properties.b_date_harvest_default} is not a valid calendar date.`, + ) + } +}
208-211: Year validation logic is sound.The bounds checking (1970-2100) and integer validation ensure reasonable year inputs. The explicit
=== falsecomparison is clear, though!Number.isInteger(year)would be more idiomatic.Optional simplification:
-if (!year || Number.isInteger(year) === false || year < 1970 || year >= 2100) { +if (!year || !Number.isInteger(year) || year < 1970 || year >= 2100) { throw new Error("Invalid year") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/cold-jars-marry.md(1 hunks)fdm-core/src/cultivation.d.ts(1 hunks)fdm-core/src/cultivation.ts(4 hunks)fdm-data/src/cultivations/catalogues/brp.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-data/src/cultivations/catalogues/brp.ts
🧰 Additional context used
🧬 Code graph analysis (1)
fdm-core/src/cultivation.ts (4)
fdm-core/src/db/schema.ts (1)
cultivationsCatalogue(356-381)fdm-core/src/index.ts (3)
getDefaultDatesOfCultivation(40-40)FdmType(72-72)PrincipalId(25-25)fdm-core/src/cultivation.d.ts (1)
CultivationDefaultDates(62-65)fdm-core/src/authorization.ts (1)
checkPermission(156-240)
🔇 Additional comments (3)
fdm-core/src/cultivation.d.ts (1)
62-65: LGTM!The type definition correctly models the default dates structure with a required start date and optional end date.
fdm-core/src/cultivation.ts (2)
20-20: LGTM!The import of
CultivationDefaultDatesand the addition of default date parameters toaddCultivationToCatalogueproperly extend the API surface to support default cultivation dates.Also applies to: 123-124
254-258: Verify source data default dates instead of silent fallbacks.The PR intends every cultivation catalogue entry to include
b_lu_start_defaultandb_date_harvest_default. I found no explicitnullcases in code, but please confirm your source data always populates these fields—fallbacks to"03-15"/"09-15"may mask missing defaults. Consider validating or throwing an error when defaults are absent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
fdm-core/src/cultivation.ts (1)
254-283: Harden date construction; prevent Invalid Date and timezone drift.Validate MM‑dd strings and build dates with UTC to avoid runtime NaN and local‑TZ shifts. Also adjusts previous‑year logic deterministically.
- // Construct the default start date using the provided year. - const cultivationDefaultDates: CultivationDefaultDates = { - b_lu_start: new Date(`${year}-${defaultStart}`), - b_lu_end: undefined, - } + // Construct and validate dates in UTC + const parseMmDdUtc = (mmdd: string, y: number) => { + const [mmS, ddS] = mmdd.split("-") + const mm = parseInt(mmS, 10) + const dd = parseInt(ddS, 10) + const d = new Date(Date.UTC(y, mm - 1, dd)) + if (d.getUTCMonth() !== mm - 1 || d.getUTCDate() !== dd) { + throw new Error(`Invalid date components '${mmdd}'`) + } + return d + } + const cultivationDefaultDates: CultivationDefaultDates = { + b_lu_start: parseMmDdUtc(defaultStart, year), + b_lu_end: undefined, + } @@ - if ( - cultivationsCatalogue[0].b_lu_harvestable === "once" && - defaultEnd - ) { - cultivationDefaultDates.b_lu_end = new Date(`${year}-${defaultEnd}`) + if (cultivationsCatalogue[0].b_lu_harvestable === "once" && defaultEnd) { + cultivationDefaultDates.b_lu_end = parseMmDdUtc(defaultEnd, year) @@ - if ( - cultivationDefaultDates.b_lu_end.getTime() <= - cultivationDefaultDates.b_lu_start.getTime() - ) { - cultivationDefaultDates.b_lu_start = new Date( - `${year - 1}-${defaultStart}`, - ) - } + if ( + cultivationDefaultDates.b_lu_end.getTime() <= + cultivationDefaultDates.b_lu_start.getTime() + ) { + cultivationDefaultDates.b_lu_start = parseMmDdUtc( + defaultStart, + year - 1, + ) + }
🧹 Nitpick comments (5)
.changeset/cold-jars-marry.md (1)
5-5: Clarify stored format in description.Consider mentioning the MM-dd format explicitly to align expectations with schema constraints and tests.
-Adds `b_lu_start_default` and `b_date_harvest_default` to cultivations catalogue as the default start and harvest dates of a cultivation +Adds `b_lu_start_default` and `b_date_harvest_default` (MM-dd) to cultivations catalogue as the default start and harvest dates of a cultivationfdm-core/src/db/schema.ts (1)
382-392: Strengthen CHECKs to validate real calendar dates (not just format).The regex enforces MM-dd shape but allows impossible dates (e.g., 02-31). Prefer a normalization-based check so only valid month/day combos pass.
Apply this pattern (uses Postgres to_date normalization and equality check; 2000 is a leap year):
- check( - "b_lu_start_default_format", - sql`b_lu_start_default IS NULL OR b_lu_start_default ~ '^(0[1-9]|1[0-2])-(0[1-9]|[12][0-9]|3[01])$'`, - ), - check( - "b_date_harvest_default_format", - sql`b_date_harvest_default IS NULL OR b_date_harvest_default ~ '^(0[1-9]|1[0-2])-(0[1-9]|[12][0-9]|3[01])$'`, - ), + check( + "b_lu_start_default_valid", + sql`b_lu_start_default IS NULL OR to_char(to_date('2000-' || b_lu_start_default, 'YYYY-MM-DD'), 'MM-DD') = b_lu_start_default`, + ), + check( + "b_date_harvest_default_valid", + sql`b_date_harvest_default IS NULL OR to_char(to_date('2000-' || b_date_harvest_default, 'YYYY-MM-DD'), 'MM-DD') = b_date_harvest_default`, + ),Optionally keep the regex CHECKs alongside for clearer error messages.
Based on learningsfdm-core/src/cultivation.test.ts (1)
830-906: Add a test for impossible MM‑dd values (e.g., 02‑31).Current tests catch format errors but not invalid day/month combos that create Invalid Date at runtime. Add a case expecting rejection when b_lu_start_default or b_date_harvest_default is "02-31".
Would you like me to draft the vitest case?
Also applies to: 907-931
fdm-core/src/cultivation.ts (2)
145-162: Validate calendar-validity, not only format, in catalogue insertion.Regex allows impossible dates (e.g., 02-31). Validate components to prevent storing bad defaults.
- // Validate if b_lu_start_default and b_date_harvest_default follows format MM-dd - const dateRegex = /^(0[1-9]|1[0-2])-(0[1-9]|[1-2][0-9]|3[0-1])$/ - if ( - properties.b_lu_start_default && - !dateRegex.test(properties.b_lu_start_default) - ) { - throw new Error( - "Invalid b_lu_start_default format. Expected MM-dd.", - ) - } - if ( - properties.b_date_harvest_default && - !dateRegex.test(properties.b_date_harvest_default) - ) { - throw new Error( - "Invalid b_date_harvest_default format. Expected MM-dd.", - ) - } + // Validate MM-dd format and calendar-validity + const isValidMmDd = (s: string) => { + const m = /^(\d{2})-(\d{2})$/.exec(s) + if (!m) return false + const mm = parseInt(m[1], 10) + const dd = parseInt(m[2], 10) + if (mm < 1 || mm > 12) return false + const d = new Date(Date.UTC(2000, mm - 1, dd)) // 2000 is leap year + return ( + d.getUTCMonth() === mm - 1 && d.getUTCDate() === dd + ) + } + if ( + properties.b_lu_start_default && + !isValidMmDd(properties.b_lu_start_default) + ) { + throw new Error( + "Invalid b_lu_start_default. Expected valid MM-dd.", + ) + } + if ( + properties.b_date_harvest_default && + !isValidMmDd(properties.b_date_harvest_default) + ) { + throw new Error( + "Invalid b_date_harvest_default. Expected valid MM-dd.", + ) + }
174-190: Doc comment nit: wording.“felt into the previous calendar year” → “fell into the previous calendar year”.
- * date and adjusts the year if the sowing date felt into the previous calendar year. + * date and adjusts the year if the sowing date fell into the previous calendar year.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
fdm-core/src/db/migrations/0014_ancient_ezekiel_stane.sqlis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/meta/0014_snapshot.jsonis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/meta/_journal.jsonis excluded by!fdm-core/src/db/migrations/**
📒 Files selected for processing (4)
.changeset/cold-jars-marry.md(1 hunks)fdm-core/src/cultivation.test.ts(15 hunks)fdm-core/src/cultivation.ts(4 hunks)fdm-core/src/db/schema.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-24T11:39:57.805Z
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-core/src/cultivation.ts:625-671
Timestamp: 2025-01-24T11:39:57.805Z
Learning: Every cultivation has an associated `cultivationTerminating` record created by `addCultivation`, with `b_terminate_date` initially set to null. Therefore, `updateCultivation` can safely assume the record exists and only needs to handle updates.
Applied to files:
fdm-core/src/cultivation.test.ts
📚 Learning: 2025-01-23T15:18:57.212Z
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-core/src/db/schema.ts:407-426
Timestamp: 2025-01-23T15:18:57.212Z
Learning: In the farm data model, each cultivation (b_lu) can have only one termination date but can have multiple harvest dates. This is enforced through the database schema where cultivationTerminating uses b_lu as primary key while cultivationHarvesting uses a composite primary key of b_id_harvestable and b_lu.
Applied to files:
fdm-core/src/cultivation.test.ts
🧬 Code graph analysis (2)
fdm-core/src/cultivation.test.ts (2)
fdm-core/src/cultivation.ts (2)
addCultivationToCatalogue(105-172)getDefaultDatesOfCultivation(191-294)fdm-core/src/index.ts (2)
addCultivationToCatalogue(39-39)getDefaultDatesOfCultivation(40-40)
fdm-core/src/cultivation.ts (4)
fdm-core/src/db/schema.ts (3)
cultivationsCatalogueTypeInsert(397-398)cultivationsCatalogueTypeSelect(395-396)cultivationsCatalogue(358-393)fdm-core/src/index.ts (3)
getDefaultDatesOfCultivation(40-40)FdmType(72-72)PrincipalId(25-25)fdm-core/src/cultivation.d.ts (1)
CultivationDefaultDates(62-65)fdm-core/src/authorization.ts (1)
checkPermission(156-240)
🔇 Additional comments (1)
fdm-core/src/cultivation.test.ts (1)
932-1106: Good coverage for default-date scenarios.Tests exercise single-/multi-/none-harvest and cross-year logic well. LGTM.
gerardhros
left a comment
gerardhros
left a comment
There was a problem hiding this comment.
LGTM.
is saw that b_start is always set to 1 jan. for now its fine given the design of calender, but that might change in the future (not in coming year).
2 participants
Summary by CodeRabbit
New Features
Data
Tests
Chores
Closes #290