Add comprehensive support for fertilizer application methods and metadata#168
Conversation
…s about fertilizer parameters
…tuitive and clear
…to represent the possible methods that can be used to apply the fertilizer
…as changed that the other properties are updated as well
🦋 Changeset detectedLatest commit: a2bd823 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 |
gerardhros
left a comment
gerardhros
left a comment
There was a problem hiding this comment.
nice work. see a few additions that might be added in next PR. also check with Brent a mapping in application methods. if it can be fully mapped then its ok, otherwise we might need to expand application option list.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
fdm-core/src/fertilizer.ts (1)
150-163:p_typeis persisted although it is not a DB column – will break the insert
propertiesis spread straight intoinput.
Becausepropertiescontains the synthetic fieldp_type, the resulting object now has a key that Drizzle cannot map to a column.
At runtime this yields “column «p_type» of relation «fertilizersCatalogue» does not exist” (PostgreSQL) and the whole insert fails.-const input: schema.fertilizersCatalogueTypeInsert = { - ...properties, - p_id_catalogue: p_id_catalogue, - p_source: b_id_farm, - hash: null, - p_type_manure: properties.p_type === "manure", - p_type_mineral: properties.p_type === "mineral", - p_type_compost: properties.p_type === "compost", -} +const { p_type, ...rest } = properties // strip pseudo-column +const input: schema.fertilizersCatalogueTypeInsert = { + ...rest, + p_id_catalogue: p_id_catalogue, + p_source: b_id_farm, + hash: null, + p_type_manure: p_type === "manure", + p_type_mineral: p_type === "mineral", + p_type_compost: p_type === "compost", +}Identical treatment is needed in the update path below.
🧹 Nitpick comments (3)
fdm-core/src/fertilizer.ts (2)
608-621: Duplicate flag-to-enum mapping deserves a helperThe
boolean ⇒ string | nullmapping logic now lives in three places (getFertilizer,getFertilizers,updateFertilizerFromCataloguetest helper). Extracting it into a tiny util prevents drift:function booleanFlagsToType(man: boolean, min: boolean, com: boolean) : "manure" | "mineral" | "compost" | null { if (man) return "manure" if (min) return "mineral" if (com) return "compost" return null }…and reuse everywhere.
980-1220: Metadata table contains several typos & copy-paste errorsQuick wins that will confuse UI consumers:
"Catalogu ID van meststof"→ “Catalogus ID …”p_na_rtdescription says “Calcium” instead of “Natrium”.p_zn_rt,p_co_rtdescriptions copy “Koper”.p_mo_rtunit showsmg Mn/kg.
These are user-visible strings, please double-check before releasing.fdm-core/src/fertilizer.test.ts (1)
1036-1076: Hard-coding22makes the spec brittleThe parameter list will inevitably grow. A safer test is to assert at least the expected core items or to snapshot the keys. Example:
-expect(descriptions).toHaveLength(22) +expect(descriptions.length).toBeGreaterThanOrEqual(22)This still protects against an empty/undefined result without blocking genuine additions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.fertilizer.tsx(5 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.fertilizers.tsx(6 hunks)fdm-app/package.json(3 hunks)fdm-core/src/fertilizer.test.ts(17 hunks)fdm-core/src/fertilizer.ts(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- fdm-app/package.json
- fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.fertilizer.tsx
- fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.fertilizers.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
fdm-core/src/fertilizer.test.ts (2)
fdm-core/src/fertilizer.ts (2)
getFertilizer(261-367)getFertilizerParametersDescription(980-1220)fdm-core/src/index.ts (2)
getFertilizer(48-48)getFertilizerParametersDescription(55-55)
⏰ Context from checks skipped due to timeout of 300000ms (2)
- GitHub Check: core (22)
- GitHub Check: calculator (22)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/fluffy-steaks-follow.md (1)
5-5: Nit: refine summary wording for clarityThe preposition “at” is a bit off here. Consider changing to:
-Fix unit conversion at calculation of N supply by other fertilizers +Fix unit conversion in calculation of N supply for other fertilizers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/fluffy-steaks-follow.md(1 hunks)fdm-app/package.json(3 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/compost.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/manure.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- fdm-app/package.json
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/compost.ts
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.ts
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/manure.ts
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/fluffy-steaks-follow.md
[uncategorized] ~5-~5: The preposition ‘for’ seems more likely in this position.
Context: ...ulator": patch --- Fix unit conversion at calculation of N supply by other fertil...
(AI_HYDRA_LEO_REPLACE_AT_FOR)
⏰ Context from checks skipped due to timeout of 300000ms (3)
- GitHub Check: data (22)
- GitHub Check: calculator (22)
- GitHub Check: core (22)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
fdm-core/src/fertilizer.ts (2)
460-477: Excellent fixes for previously identified issues.This properly addresses both past review comments:
- Destructuring
p_typeprevents pseudo column leak- Conditional logic preserves existing flags when
p_typeis not providedWell done on implementing the suggested fixes!
479-481: Same type casting concern as in addFertilizerToCatalogue.The type casting to
CatalogueFertilizerItemshould be verified for compatibility with the union type structure.
🧹 Nitpick comments (1)
fdm-core/src/fertilizer.ts (1)
609-622: Consider extracting type conversion logic to reduce duplication.This type conversion from boolean flags to union type is identical to the logic in
getFertilizer. Consider extracting this to a utility function to follow DRY principles.+// Add utility function at the top of the file +function convertFertilizerType( + p_type_manure: boolean, + p_type_mineral: boolean, + p_type_compost: boolean +): "manure" | "mineral" | "compost" | null { + if (p_type_manure) return "manure" + if (p_type_mineral) return "mineral" + if (p_type_compost) return "compost" + return null +} // Then use in both places: return fertilizers.map((f) => ({ ...f, - p_type: f.p_type_manure ? "manure" : f.p_type_mineral ? "mineral" : f.p_type_compost ? "compost" : null, + p_type: convertFertilizerType(f.p_type_manure, f.p_type_mineral, f.p_type_compost), }))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts(2 hunks)fdm-core/src/fertilizer.ts(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
fdm-core/src/fertilizer.ts (4)
fdm-core/src/db/schema.ts (2)
fertilizersCatalogueTypeInsert(245-246)fertilizers(113-121)fdm-data/src/fertilizers/hash.ts (1)
hashFertilizer(4-28)fdm-core/src/index.ts (2)
getFertilizerParametersDescription(55-55)FertilizerParameterDescription(61-61)fdm-core/src/fertilizer.d.ts (1)
FertilizerParameterDescription(121-122)
⏰ Context from checks skipped due to timeout of 300000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: calculator (22)
- GitHub Check: core (22)
🔇 Additional comments (6)
fdm-core/src/fertilizer.ts (6)
4-4: Import additions look good.The new imports for
CatalogueFertilizerItemandhashFertilizerare necessary for the enhanced functionality.
96-96: Good addition of application method options parameter.This new parameter aligns with the PR objectives for comprehensive fertilizer application method support.
137-137: Excellent refactoring of fertilizer type representation.Using a union type instead of separate boolean parameters is much cleaner and more type-safe.
344-361: Excellent error handling and type conversion improvements.The explicit error throwing for not found cases is much better than implicit undefined returns. The type conversion logic correctly maps the boolean flags back to the cleaner union type representation.
1046-1046: I’m going to search the entire repo for any definition or export ofapplicationMethodOptionsto confirm its availability.#!/bin/bash # 1. Find all usages/definitions of applicationMethodOptions rg -n 'applicationMethodOptions' # 2. Check specifically for any export statement rg -n 'export.*applicationMethodOptions'
160-162: Let’s pull in the fullCatalogueFertilizerItemdefinition and check for anyp_typefield:#!/bin/bash # Show the complete interface for CatalogueFertilizerItem rg -n -A20 "export interface CatalogueFertilizerItem" fdm-data/src/fertilizers/d.ts # Verify whether `p_type` appears in this interface rg -n -A2 -B2 "p_type" fdm-data/src/fertilizers/d.ts
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
fdm-core/src/fertilizer.ts (3)
390-390: Past review comments properly addressed.The implementation correctly resolves previous issues:
- Destructures
p_typeto prevent pseudo column leakage- Preserves existing type flags when
p_typeis undefined- Properly calculates hash for updated properties
Also applies to: 431-431, 460-481
981-984: Locale validation could be more robust.The locale validation only checks for exact string match and doesn't provide informative error messages about supported locales.
Consider implementing the previously suggested improvement:
export function getFertilizerParametersDescription( locale = "NL-nl", ): FertilizerParameterDescription { - if (locale !== "NL-nl") throw new Error("Unsupported locale") + const supportedLocales = ["NL-nl"] as const + if (!supportedLocales.includes(locale as any)) { + throw new Error(`Unsupported locale: ${locale}. Supported locales: ${supportedLocales.join(", ")}`) + }
1154-1154: Previously reported parameter description errors have been resolved.The copy-paste errors in parameter descriptions have been correctly fixed:
- Na: "Natrium" ✓
- Zn: "Zink" ✓
- Co: "Kobalt" ✓
- Mo unit: "mg Mo/kg" ✓
Also applies to: 1174-1174, 1184-1184, 1200-1200
🧹 Nitpick comments (1)
fdm-core/src/fertilizer.ts (1)
1010-1025: Consider addressing maintainability and completeness concerns.Several areas could be improved:
- Commented out parameter descriptions suggest incomplete implementation
- Empty description fields reduce metadata value (e.g., lines 1054, 1064)
- Schema reference
schema.applicationMethodOptionsshould be validated to ensure it exists- The large hardcoded array may impact maintainability as the system grows
Consider:
- Completing or removing commented sections
- Adding meaningful descriptions for empty fields
- Validating schema references
- Externalizing parameter metadata to a configuration file for easier maintenance
Also applies to: 1046-1046, 1054-1054, 1064-1064
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fdm-core/src/fertilizer.ts(12 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
fdm-core/src/fertilizer.ts (3)
fdm-data/src/fertilizers/hash.ts (1)
hashFertilizer(4-28)fdm-core/src/index.ts (2)
getFertilizerParametersDescription(55-55)FertilizerParameterDescription(61-61)fdm-core/src/fertilizer.d.ts (1)
FertilizerParameterDescription(121-122)
⏰ Context from checks skipped due to timeout of 300000ms (2)
- GitHub Check: core (22)
- GitHub Check: calculator (22)
🔇 Additional comments (4)
fdm-core/src/fertilizer.ts (4)
4-4: New imports support enhanced fertilizer functionality correctly.The added imports for
hashFertilizer,CatalogueFertilizerItem, andFertilizerParameterDescriptionare properly utilized throughout the file for hash calculation and parameter metadata features.Also applies to: 10-14
96-96: Fertilizer properties and hash calculation implemented correctly.The new
p_app_method_optionsand unifiedp_typeparameters are properly integrated. The boolean flag mapping logic correctly sets the appropriate type flags, and the hash calculation follows the established pattern.Also applies to: 137-137, 156-162
275-276: Enhanced error handling and type conversion logic.Good improvements: explicit error throwing when fertilizer is not found and correct conversion from boolean flags to the unified
p_typefield. The logic properly handles all cases including when no type flags are set.Also applies to: 344-361
540-541: Consistent type conversion across fertilizer retrieval functions.The mapping logic correctly applies the same boolean-to-union type conversion used in
getFertilizer, ensuring consistentp_typerepresentation across all retrieval functions.Also applies to: 609-622
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Chores