Add cultivation variety support across DB, APIs, data, app#237
Conversation
… varieties for a cultivation
…of a cultivation and `b_lu_variety_options` to list the possible varieties of a cultivation
… this cultivation, to select a variety
…rom field update the information in the cultivation details card as well
🦋 Changeset detectedLatest commit: a5be135 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #237 +/- ##
===============================================
+ Coverage 92.86% 92.94% +0.07%
===============================================
Files 85 85
Lines 12889 13004 +115
Branches 1294 1310 +16
===============================================
+ Hits 11970 12086 +116
+ Misses 917 916 -1
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:
|
WalkthroughAdds per-cultivation variety support across DB, core, data and app: new fields Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Loader
participant Catalogue
participant UI
participant Action
participant Core
User->>Loader: GET cultivation page
Loader->>Catalogue: getCultivationsFromCatalogue()
Catalogue-->>Loader: catalogue incl. b_lu_variety_options
Loader->>UI: props { cultivation, b_lu_variety_options }
User->>UI: select variety + submit
UI->>Action: POST { b_lu_variety, ... }
Action->>Core: updateCultivation(..., b_lu_variety)
Core-->>Action: ok / validation error
Action-->>User: response / redirect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
Bot
changed the title
coderabbitai
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (4)
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop._index.tsx (2)
187-193: Remove unused props passed to CultivationDetailsCardThe card component no longer takes harvests or b_lu_harvestable. Passing them will cause a TS error. Drop them here.
<CultivationDetailsCard cultivation={loaderData.cultivation} - harvests={loaderData.harvests} - b_lu_harvestable={loaderData.b_lu_harvestable} b_lu_variety_options={loaderData.b_lu_variety_options} />
254-264: Bug: variety and crop residue changes are not saved unless dates changeThe update is currently guarded by checking only b_lu_start/b_lu_end. If the user only updates b_lu_variety or m_cropresidue, updateCultivation is never called. Include those fields in the condition or remove the guard.
Option A (simple and safe): always call updateCultivation.
- if (formValues.b_lu_start || formValues.b_lu_end) { - await updateCultivation( - fdm, - session.principal_id, - item, - undefined, - formValues.b_lu_start, - formValues.b_lu_end, - formValues.m_cropresidue, - formValues.b_lu_variety, - ) - } + await updateCultivation( + fdm, + session.principal_id, + item, + undefined, + formValues.b_lu_start, + formValues.b_lu_end, + formValues.m_cropresidue, + formValues.b_lu_variety, + )Option B (conditional): gate on presence of any field that could change.
- if (formValues.b_lu_start || formValues.b_lu_end) { + if ( + formValues.b_lu_start !== undefined || + formValues.b_lu_end !== undefined || + formValues.m_cropresidue !== undefined || + formValues.b_lu_variety !== undefined + ) { await updateCultivation( fdm, session.principal_id, item, undefined, formValues.b_lu_start, formValues.b_lu_end, formValues.m_cropresidue, formValues.b_lu_variety, ) }Pick one based on your desired write behavior. I recommend Option B if you want to avoid unnecessary writes.
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation.$b_lu.tsx (1)
241-253: Cannot clear a previously set variety; normalize and pass null to clearCurrently, if the user clears the select, many form libs submit "" which is falsy and will be ignored by updateCultivation (cannot clear). Normalize empty strings to null and always pass the param so the backend can clear it.
- const { b_lu_start, b_lu_end, m_cropresidue, b_lu_variety } = - formValues + const { b_lu_start, b_lu_end, m_cropresidue, b_lu_variety } = + formValues + const normalized_b_lu_variety = + typeof b_lu_variety === "string" && b_lu_variety.trim() === "" + ? null + : b_lu_variety ?? null @@ await updateCultivation( fdm, session.principal_id, b_lu, undefined, b_lu_start, b_lu_end, m_cropresidue, - b_lu_variety, + normalized_b_lu_variety, )Also ensure CultivationDetailsFormSchema allows null/undefined for b_lu_variety.
fdm-core/src/cultivation.ts (1)
737-745: Normalize b_lu_variety before grouping to avoid duplicate “no variety” bucketsIt doesn’t look like empty strings or
nullvalues forb_lu_varietyare being normalized anywhere—so rows with""will not match rows withnull. To prevent logically identical “no variety” entries from ending up in separate groups, normalizeb_lu_varietyto a single canonical value (e.g.null) before you callreduce.Suggested change in cultivation.ts around line 740:
- let existingCultivation = acc.find( + // Normalize variety to null so "" and null don’t split into separate buckets + const varietyKey = curr.b_lu_variety ?? null; + let existingCultivation = acc.find( (item) => item.b_lu_catalogue === curr.b_lu_catalogue && - item.b_lu_variety === curr.b_lu_variety && + (item.b_lu_variety ?? null) === varietyKey && (item.b_lu_start?.getTime() ?? 0) === (curr.b_lu_start?.getTime() ?? 0) && (item.b_lu_end?.getTime() ?? 0) === (curr.b_lu_end?.getTime() ?? 0), )– In the same reduce, replace all uses of
curr.b_lu_varietyanditem.b_lu_varietywith your normalizedvarietyKeyor(item.b_lu_variety ?? null)as shown above.
– This ensures that both""andnullmap to the same group key.
– Run your tests to confirm no existing behavior changes.
🧹 Nitpick comments (19)
.changeset/dirty-jeans-rhyme.md (1)
5-5: Polish the changeset description for clarity and grammarThe sentence is missing “to” and reads awkwardly. Suggested rewording below.
-Add `CultivationDetailsCard` enable the user, if applicable for this cultivation, to select a variety +Add `CultivationDetailsCard` to enable the user, when applicable for this cultivation, to select a variety..changeset/smart-keys-post.md (1)
5-5: Clarify the changeset descriptionCurrent phrasing is hard to parse. Suggested, more precise wording:
-When using the header at cultivations page of a field to switch from field update the information in the cultivation details card as well +When using the header on a field’s cultivations page to switch fields, also update the information shown in the cultivation details card..changeset/common-pens-crash.md (1)
5-5: Disambiguate where the new field is added“to cultivations” is ambiguous. The field lives on cultivation catalogue items (and is mirrored in data typings). Suggest:
-Add `b_lu_variety_options` to cultivations to list the possible varieties for a cultivation +Add `b_lu_variety_options` to cultivation catalogue items to list the possible varieties for a cultivation.fdm-core/src/db/schema.ts (2)
296-296: Optional b_lu_variety matches requirements; consider documenting constraint semanticsMaking
b_lu_varietyoptional aligns with Issue #191 (optional field) and the app’s flow. Two follow-ups:
- Document that, when set, its value must be one of the catalogue’s
b_lu_variety_options(enforced in application logic).- If you expect filtering by variety, consider adding an index later.
You can inline a short comment for future maintainers:
- b_lu_variety: text(), + b_lu_variety: text(), // optional; when set, must be one of b_lu_variety_options for the selected catalogue itemPlease verify that:
addCultivation/updateCultivationreject values not present inb_lu_variety_options.- Migrations include adding this column (nullable).
373-373: Null vs empty array semantics for variety options should be explicitThe array column is correct. Ensure consistent semantics across layers:
- null: no varieties defined/applicable for this cultivation.
- []: applicable but currently no choices (should this state exist? If not, prefer null over empty array).
Add a clarifying comment:
- b_lu_variety_options: text().array(), + b_lu_variety_options: text().array(), // null => not applicable; non-null array => allowed options (values for b_lu_variety)Please confirm the UI and validation code treat null distinctly from an empty array and that tests cover both cases (they seem to via
nulland["Agria"]).fdm-core/src/harvest.test.ts (5)
124-127: Add real cleanup or rollback to ensure test isolationThe afterAll block is currently a placeholder. Either:
- Wrap each test in a transaction and roll back in afterEach, or
- Delete created rows in reverse FK order.
Given prior learnings (don’t call
fdm.end(); usefdm.delete(schema.table).execute()), consider adding targeted cleanup. If you keep afterAll, at minimum clean rows related tob_lucreated in this suite.Example skeleton (adjust ordering/filters to your FK setup):
// Example cleanup (adjust as needed) // Note: delete children before parents to satisfy FKs. const lusToClean = [b_lu /* push any additional b_lu created during tests */] // Delete harvest-related rows for our b_lu's for (const lu of lusToClean) { // Remove sampling and analyses by joining via harvesting rows const harvestingRows = await fdm .select({ b_id_harvestable: schema.cultivationHarvesting.b_id_harvestable, b_id_harvesting: schema.cultivationHarvesting.b_id_harvesting }) .from(schema.cultivationHarvesting) .where(eq(schema.cultivationHarvesting.b_lu, lu)) for (const row of harvestingRows) { await fdm.delete(schema.harvestableSampling).where(eq(schema.harvestableSampling.b_id_harvestable, row.b_id_harvestable)).execute() await fdm.delete(schema.harvestableAnalyses).where(eq(schema.harvestableAnalyses.b_id_harvestable, row.b_id_harvestable)).execute() } await fdm.delete(schema.cultivationHarvesting).where(eq(schema.cultivationHarvesting.b_lu, lu)).execute() await fdm.delete(schema.cultivationEnding).where(eq(schema.cultivationEnding.b_lu, lu)).execute() await fdm.delete(schema.cultivations).where(eq(schema.cultivations.b_lu, lu)).execute() } // Consider cleaning created field/farm if they are test-specific and not sharedAlternatively, consider a per-test transaction helper that creates data and rolls back in afterEach.
238-240: Make assertion resilient to message changesAsserting the full permission error string is brittle. Match by substring/regex instead.
- ).rejects.toThrowError( - "Principal does not have permission to perform this action", - ) + ).rejects.toThrow(/permission/i)
361-368: Strengthen assertion: verify end date remains null for multiple-harvest cultivations“not equal to newHarvestDate” is weaker than asserting the business invariant (no end date). If the end date was unset before, assert it remains null.
- expect(cultivation[0].b_lu_end).not.toEqual(newHarvestDate) + expect(cultivation[0].b_lu_end).toBeNull()If there are paths that could set an end date earlier (e.g., manual termination), adjust the test to set it explicitly and assert it remains unchanged instead of null.
143-153: Minor: also assert analyses array length for clarityYou already validate yield; consider asserting that exactly one analysis exists for the newly added harvest to make the intent explicit.
expect(newHarvest).toBeDefined() expect(newHarvest?.b_lu_harvest_date).toEqual(harvestDate) - expect( - newHarvest?.harvestable.harvestable_analyses[0].b_lu_yield, - ).toEqual(yieldValue) + expect(newHarvest?.harvestable.harvestable_analyses.length).toBe(1) + expect(newHarvest?.harvestable.harvestable_analyses[0].b_lu_yield).toEqual(yieldValue)
329-368: Optional: add a test for getHarvests timeframe filteringSince getHarvests supports an optional timeframe, consider adding a test that:
- Inserts multiple harvests on different dates, then
- Queries with a timeframe that includes a subset and asserts only those are returned.
I can propose a ready-to-drop test if you confirm the timeframe parameter shape (start/end types) used in getHarvests.
fdm-data/src/cultivations/d.ts (1)
30-30: Clarify null vs empty-array semantics for b_lu_variety_optionsUsing string[] | null is consistent with other nullable fields here. Please confirm the loader/builders always set this to null when there are no options (not []), and trim/filter empty strings when splitting the source value (e.g., "a||b" → ["a","b"]), to avoid spurious hash changes and UI ambiguity.
Optionally, consider marking the array as readonly to communicate immutability:
- b_lu_variety_options: readonly string[] | null
fdm-core/src/cultivation.d.ts (1)
12-12: Confirm nullability semantics for b_lu_variety across APITyping b_lu_variety as schema.cultivationsTypeSelect["b_lu_variety"] is correct and will reflect nullability from the DB. Please confirm:
- The DB column is nullable (optional field per objective #191).
- API responses consistently include the property set to null when not chosen (rather than omitting it), to keep the type stable for consumers.
Optional: add a short JSDoc to both interfaces to document expected behavior (null when not applicable).
Also applies to: 22-22
fdm-data/src/cultivations/hash.test.ts (1)
200-229: Nit: test title grammar and consider order-sensitivity test
- Title: “a array” → “an array”.
- Optional: If the catalogue treats variety options as an unordered set, consider adding a test to clarify expected behavior for order differences (e.g., ["A","B"] vs ["B","A"]). Current hasher is order-sensitive for arrays.
Apply this diff for the test title:
- it("should generate different hashes when a array of strings value is changed", async () => { + it("should generate different hashes when an array of strings value is changed", async () => {If you want me to add an order-sensitivity test, say so and I’ll draft it.
fdm-app/app/components/blocks/cultivation/card-details.tsx (1)
167-173: Nit: fix spelling in placeholder text“Geen varieteiten beschikbaar” → “Geen variëteiten beschikbaar”.
- ? "Geen varieteiten beschikbaar" + ? "Geen variëteiten beschikbaar"fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation.$b_lu.tsx (2)
148-152: Type mismatch: find() returns CultivationCatalogue, not CultivationCatalogueItemYou're finding items from cultivationsCatalogue: CultivationCatalogue[], but annotating the result as CultivationCatalogueItem | undefined. Unless these types are identical (structurally compatible), this will cause a TS error or a fragile assumption. Prefer inference or align the type explicitly to CultivationCatalogue.
Apply this diff to let TS infer the correct type (or align it to CultivationCatalogue):
- const cultivationCatalogueItem: CultivationCatalogueItem | undefined = - cultivationsCatalogue.find((item) => { + const cultivationCatalogueItem = + cultivationsCatalogue.find((item) => { return item.b_lu_catalogue === cultivation.b_lu_catalogue })
154-162: Gracefully handle empty or null variety optionsIf b_lu_variety_options is null or an empty array, the current mapping yields [] (good). Consider trimming values to avoid UI duplicates due to whitespace and ensure stable labeling.
- if (cultivationCatalogueItem.b_lu_variety_options) { + if (cultivationCatalogueItem.b_lu_variety_options?.length) { b_lu_variety_options = cultivationCatalogueItem.b_lu_variety_options.map( - (option: string) => ({ - value: option, - label: option, + (option: string) => { + const o = option.trim() + return { + value: o, + label: o, + } }), ) }fdm-core/src/cultivation.test.ts (1)
737-738: Type shadowing of fdm across suites is intentional but can be confusingYou re-declare fdm as FdmType in this suite, while the previous suite uses FdmServerType. If FdmType and FdmServerType are assignment-compatible this is fine, but consider aligning to one type alias for consistency across tests.
Would you like me to unify the test typing (FdmType) across both suites?
fdm-core/src/cultivation.ts (2)
342-348: Error context missing b_lu_varietyWhen addCultivation fails on variety validation, b_lu_variety is not included in the error context, which can slow debugging. Consider adding it.
throw handleError(err, "Exception for addCultivation", { b_lu_catalogue, b_id, b_lu_start, b_lu_end, + b_lu_variety, })
1143-1149: Error context missing b_lu_variety in updateFor parity with addCultivation and easier diagnostics, include b_lu_variety in the update error context.
throw handleError(err, "Exception for updateCultivation", { b_lu, b_lu_catalogue, b_lu_start, b_lu_end, + b_lu_variety, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
fdm-core/src/db/migrations/0013_jittery_warbird.sqlis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/meta/0013_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 (18)
.changeset/angry-pugs-grab.md(1 hunks).changeset/common-pens-crash.md(1 hunks).changeset/dirty-jeans-rhyme.md(1 hunks).changeset/smart-keys-post.md(1 hunks)fdm-app/app/components/blocks/cultivation/card-details.tsx(7 hunks)fdm-app/app/components/blocks/cultivation/schema.ts(1 hunks)fdm-app/app/components/blocks/header/field.tsx(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation.$b_lu.tsx(7 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop._index.tsx(6 hunks)fdm-core/src/catalogues.test.ts(1 hunks)fdm-core/src/cultivation.d.ts(2 hunks)fdm-core/src/cultivation.test.ts(15 hunks)fdm-core/src/cultivation.ts(13 hunks)fdm-core/src/db/schema.ts(2 hunks)fdm-core/src/harvest.test.ts(8 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(9 hunks)
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2024-11-27T11:27:27.797Z
Learnt from: SvenVw
PR: SvenVw/fdm#9
File: fdm-data/src/cultivations/index.test.ts:57-59
Timestamp: 2024-11-27T11:27:27.797Z
Learning: When cleaning up test data in `afterAll` hooks in `fdm-data/src/cultivations/index.test.ts`, use `fdm.delete(schema.tableName).execute();` to delete rows from a table without dropping the table itself.
Applied to files:
fdm-core/src/catalogues.test.tsfdm-core/src/harvest.test.tsfdm-core/src/cultivation.test.ts
📚 Learning: 2024-11-27T12:15:36.425Z
Learnt from: SvenVw
PR: SvenVw/fdm#9
File: fdm-data/src/cultivations/index.test.ts:57-59
Timestamp: 2024-11-27T12:15:36.425Z
Learning: In `fdm-data/src/cultivations/index.test.ts`, the `fdm` object created by `drizzle` does not have an `.end()` method. Cleanup code should not attempt to call `fdm.end();`.
Applied to files:
fdm-core/src/catalogues.test.tsfdm-core/src/harvest.test.tsfdm-core/src/cultivation.test.ts
📚 Learning: 2025-02-13T09:03:11.890Z
Learnt from: SvenVw
PR: SvenVw/fdm#71
File: fdm-app/app/routes/farm.create.$b_id_farm.cultivations.$b_lu_catalogue.crop.harvest._index.tsx:111-135
Timestamp: 2025-02-13T09:03:11.890Z
Learning: When adding multiple harvests in fdm-app, use Promise.all instead of Promise.allSettled to ensure atomic behavior - if one harvest addition fails, all should fail and rollback to maintain data consistency.
Applied to files:
fdm-core/src/catalogues.test.tsfdm-core/src/harvest.test.tsfdm-core/src/cultivation.test.ts
📚 Learning: 2025-08-11T11:55:26.034Z
Learnt from: SvenVw
PR: SvenVw/fdm#233
File: fdm-app/app/integrations/nmi.ts:54-0
Timestamp: 2025-08-11T11:55:26.034Z
Learning: The NMI API Estimates endpoint (`https://api.nmi-agro.nl/estimates`) always returns the fields `b_gwl_ghg`, `b_gwl_glg`, and `cultivations` according to its specification. These fields should be kept as required (not optional) in the TypeScript return type and Zod validation schema in `fdm-app/app/integrations/nmi.ts`.
Applied to files:
fdm-core/src/db/schema.tsfdm-core/src/cultivation.d.tsfdm-app/app/components/blocks/cultivation/schema.ts
📚 Learning: 2025-04-18T14:20:40.975Z
Learnt from: SvenVw
PR: SvenVw/fdm#124
File: fdm-core/src/db/schema-authn.ts:70-76
Timestamp: 2025-04-18T14:20:40.975Z
Learning: The organization schema in fdm-core/src/db/schema-authn.ts is managed by better-auth, and modifications to field constraints (like making the slug field non-nullable) should maintain compatibility with better-auth's expectations, even if application code assumes non-null values.
Applied to files:
fdm-core/src/db/schema.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/db/schema.tsfdm-core/src/cultivation.d.tsfdm-app/app/components/blocks/cultivation/card-details.tsxfdm-core/src/harvest.test.tsfdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop._index.tsxfdm-core/src/cultivation.test.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation.$b_lu.tsxfdm-core/src/cultivation.ts
📚 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-app/app/components/blocks/cultivation/card-details.tsxfdm-core/src/harvest.test.tsfdm-core/src/cultivation.test.tsfdm-core/src/cultivation.ts
📚 Learning: 2025-08-11T12:14:28.331Z
Learnt from: SvenVw
PR: SvenVw/fdm#233
File: fdm-core/src/db/schema.ts:370-0
Timestamp: 2025-08-11T12:14:28.331Z
Learning: In the FDM database schema, the field `b_lu_rest_oravib` in the `cultivations_catalogue` table is intentionally nullable (can be undefined). When undefined, it should be treated as `false` in the UI layer. This allows distinguishing between "explicitly set to false" and "not set" at the database level while providing simplified boolean logic in the UI.
Applied to files:
fdm-core/src/harvest.test.ts
📚 Learning: 2025-02-13T08:35:59.306Z
Learnt from: SvenVw
PR: SvenVw/fdm#71
File: fdm-app/app/routes/farm.$b_id_farm.field.$b_id.cultivation.$b_lu.harvest.$b_id_harvesting.tsx:114-124
Timestamp: 2025-02-13T08:35:59.306Z
Learning: The HarvestForm component in fdm-app expects undefined (not 0) for b_lu_yield when no yield information is available, as 0 would incorrectly imply that yield data exists.
Applied to files:
fdm-core/src/harvest.test.tsfdm-core/src/cultivation.test.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation.$b_lu.tsx
📚 Learning: 2025-02-13T08:28:00.183Z
Learnt from: SvenVw
PR: SvenVw/fdm#71
File: fdm-core/src/cultivation.ts:572-597
Timestamp: 2025-02-13T08:28:00.183Z
Learning: In the FDM database schema, `b_id_harvesting` is a primary key in the `cultivationHarvesting` table, ensuring no duplicate harvest entries can exist. Therefore, duplicate checking is not needed when processing harvest records from database queries.
Applied to files:
fdm-core/src/harvest.test.ts
📚 Learning: 2025-07-31T11:38:50.661Z
Learnt from: SvenVw
PR: SvenVw/fdm#194
File: fdm-core/src/harvest.ts:488-644
Timestamp: 2025-07-31T11:38:50.661Z
Learning: The validation logic in updateHarvest is intentionally different from checkHarvestDateCompability because updateHarvest is for updating existing harvests while checkHarvestDateCompability is for inserting new harvests. The insertion function includes checks that don't apply to updates, such as verifying no harvest already exists for "once" harvestable cultivations.
Applied to files:
fdm-core/src/harvest.test.tsfdm-core/src/cultivation.ts
📚 Learning: 2025-04-04T14:27:39.518Z
Learnt from: SvenVw
PR: SvenVw/fdm#116
File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx:111-154
Timestamp: 2025-04-04T14:27:39.518Z
Learning: In the FDM application, cultivation retrieval logic should be centralized in utility functions rather than duplicated across loader and action functions to improve maintainability and ensure consistent behavior.
Applied to files:
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation.$b_lu.tsx
📚 Learning: 2025-01-09T16:03:37.764Z
Learnt from: SvenVw
PR: SvenVw/fdm#42
File: fdm-app/app/routes/farm/_b_id_farm/layout.tsx:46-95
Timestamp: 2025-01-09T16:03:37.764Z
Learning: A shared layout component `FarmLayoutBase` has been created in `components/custom/farm-layout-base.tsx` to maintain consistency across farm-related pages. The component handles farm selection dropdown, breadcrumb navigation, and provides a common layout structure.
Applied to files:
fdm-app/app/components/blocks/header/field.tsx
🧬 Code Graph Analysis (7)
fdm-core/src/catalogues.test.ts (2)
fdm-core/src/index.ts (1)
syncCatalogues(35-35)fdm-core/src/catalogues.ts (1)
syncCatalogues(373-376)
fdm-core/src/cultivation.d.ts (1)
fdm-core/src/db/schema.ts (1)
cultivationsTypeSelect(303-303)
fdm-data/src/cultivations/hash.test.ts (2)
fdm-data/src/cultivations/d.ts (1)
CatalogueCultivationItem(3-32)fdm-data/src/cultivations/hash.ts (1)
hashCultivation(4-28)
fdm-core/src/harvest.test.ts (2)
fdm-core/src/index.ts (3)
addHarvest(104-104)getHarvests(106-106)getHarvest(105-105)fdm-core/src/harvest.ts (3)
addHarvest(43-141)getHarvests(192-267)getHarvest(155-175)
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop._index.tsx (3)
fdm-core/src/db/schema.ts (1)
cultivationsCatalogue(356-379)fdm-core/src/index.ts (2)
CultivationCatalogue(49-49)getCultivationsFromCatalogue(43-43)fdm-core/src/cultivation.ts (1)
getCultivationsFromCatalogue(41-91)
fdm-core/src/cultivation.test.ts (3)
fdm-core/src/cultivation.ts (5)
addCultivation(167-349)getCultivation(362-423)updateCultivation(935-1150)addCultivationToCatalogue(102-148)getCultivationsFromCatalogue(41-91)fdm-core/src/index.ts (6)
addCultivation(38-38)getCultivation(40-40)updateCultivation(45-45)addCultivationToCatalogue(39-39)getCultivationsFromCatalogue(43-43)FdmType(69-69)fdm-core/src/db/schema.ts (1)
cultivations(289-301)
fdm-core/src/cultivation.ts (1)
fdm-core/src/db/schema.ts (2)
cultivationsCatalogueTypeInsert(383-384)cultivationsTypeInsert(304-304)
🔇 Additional comments (19)
fdm-core/src/harvest.test.ts (1)
78-112: Good coverage of catalogue variety options: both null and non-empty cases exercisedDefining one catalogue item with
nulland another with["Agria"]is a solid way to cover the two semantics..changeset/angry-pugs-grab.md (1)
5-5: Good, concise summaryThe description accurately reflects the scope. If fdm-app is not published, no changeset entry is needed; otherwise consider a patch/minor bump there too given route/UI contract updates.
fdm-data/src/cultivations/hash.test.ts (3)
22-22: Good: fixtures updated to include new fieldIncluding b_lu_variety_options in the base fixture keeps the hash inputs aligned with the new public type. No issues.
49-49: Consistent fixture updates across testsAdding b_lu_variety_options: null consistently across all fixtures ensures deterministic hashing behavior given hash.ts filters null/undefined keys. Looks good.
Also applies to: 68-68, 94-94, 124-124, 185-185
191-198: Change driver tweak looks correctSwitching the inequality driver to b_lu_hcat3_name: null is valid and still exercises the “non-null to null” path that affects the filtered keys during hashing.
fdm-app/app/components/blocks/cultivation/card-details.tsx (1)
32-36: Prop mismatch with call sites (harvests, b_lu_harvestable)This component now only accepts { cultivation, b_lu_variety_options }, but the create route still passes harvests and b_lu_harvestable. Update call sites or widen this component’s prop type to include/ignore those props to satisfy TypeScript.
If the card shouldn’t receive these props anymore, I’ll generate a patch in the route to remove them (see my comment on the route file).
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop._index.tsx (3)
94-116: Catalogue variety options mapping looks goodFetching the catalogue and mapping string options to value/label pairs is straightforward and defers empty/null handling to the upstream data. This aligns with the DB type (text[]). No issues.
160-167: LGTM: loader return payload extended with variety optionsReturning b_lu_variety_options from the loader keeps the UI self-sufficient. Good propagation.
1-14: Heads-up: Clean up unused imports when dropping harvest props
If you later remove theharvestsorb_lu_harvestableprops from<CultivationDetailsCard>, remember to remove the now-unused imports to keep this file tidy:
CultivationHarvestsCardfrom~/components/blocks/cultivation/card-harvestsCultivationDetailsFormSchemafrom~/components/blocks/cultivation/schemaHarvestableTypefrom~/components/blocks/harvest/typesNo changes needed in the shown import block right now—just a future reminder.
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation.$b_lu.tsx (1)
195-196: Prop threading LGTMPassing b_lu_variety_options to CultivationDetailsCard is consistent with the loader payload and the new UI contract.
fdm-core/src/cultivation.test.ts (4)
614-637: Great coverage for adding with a valid varietyThis test verifies persistence of b_lu_variety end-to-end. Solid.
639-654: Good negative-path coverage for invalid variety on addCovers the core validation path and expected error envelope. LGTM.
656-670: Good negative-path coverage for invalid variety on updateThis will catch regressions in server-side validation. LGTM.
693-728: Catalogue variety options retrieval test is valuableVerifies that getCultivationsFromCatalogue exposes b_lu_variety_options. Good signal.
fdm-core/src/cultivation.ts (5)
96-101: Docstring update LGTMThe doc now mentions optional b_lu_variety_options. Clear and helpful.
119-121: Schema-typed properties are goodTyping b_lu_variety_options to the schema insert type keeps compile-time safety. LGTM.
389-390: Adding b_lu_variety to getCultivation payload is correctThis unblocks UI/consumers. LGTM.
479-480: Adding b_lu_variety to getCultivations payload is correctAPI surface now consistent across single and list fetch. LGTM.
619-651: Exposing b_lu_variety in cultivation plan is usefulIncluding variety in the plan selector and as a grouping dimension is valuable. LGTM.
|
@coderabbitai , Yes no fdm-calculator changes are present as it was already covered in fdm-calculator and with the update fdm-core functions it should be included |
|
Tip For best results, initiate chat on the files or code changes. @SvenVw Thank you for the clarification! That makes perfect sense - the fdm-calculator integration was handled separately, and with the updated fdm-core functions in this PR (like The architecture approach of updating the core data layer first, then having dependent services consume the enhanced APIs, is a clean separation of concerns. ✏️ Learnings added
|
…h catalogue and variety
BoraIneviNMI
left a comment
BoraIneviNMI
left a comment
There was a problem hiding this comment.
All looks good to me. I tried running the tests and tried to add cultivations to my farms. #191 says there needs to be some changes in fdm-calculator but I think we have discussed you would add those in another PR.
As explained in this comment #237 (comment) it was not needed to update fdm-calculator as it was already prepared for it |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Closes #191 and #219