Add farm-level batch query functions and refactor input collectors#547
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds farm-scoped batch query APIs to fdm-core and refactors multiple fdm-calculator input collectors to use those batch queries, replacing per-field/per-cultivation N+1 query loops with single farm-level fetches and in-memory assembly of per-field results. Changes
Sequence Diagram(s)sequenceDiagram
participant Collector as Collector (fdm-calculator)
participant Core as Core API (fdm-core)
participant DB as Database
Collector->>Core: getFields(principal, farmId)
Collector->>Core: Promise.all([getCultivationsForFarm, getHarvestsForFarm, getSoilAnalysesForFarm, getFertilizerApplicationsForFarm])
Core->>DB: single farm-scoped joined queries
DB-->>Core: rows
Core-->>Collector: Maps keyed by field/cultivation IDs
Collector->>Collector: assemble per-field inputs from Maps (in-memory)
Collector-->>Caller: Map<fieldId, Input> (final result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 📢 Thoughts on this report? Let us know! |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
fdm-core/src/fertilizer.test.ts (1)
1411-1691: Add a permission-denied case forgetFertilizerApplicationsForFarm.Given this API’s farm-level permission semantics, a negative auth test would protect against regressions in authorization flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/fertilizer.test.ts` around lines 1411 - 1691, Add a negative auth test for getFertilizerApplicationsForFarm: create a second principal (e.g., principal_no_access) who does NOT own or have permissions on b_id_farm, then call getFertilizerApplicationsForFarm(fdm, principal_no_access, b_id_farm) and assert it fails with the expected permission-denied behavior (rejects/throws or returns an auth error) instead of returning data; place this case alongside the other tests in the describe block and mirror setup using existing helpers (createId, addFarm, addField, addFertilizerApplication) so the test verifies farm-level authorization is enforced.fdm-calculator/src/balance/organic-matter/input.test.ts (1)
397-406: TightengetSoilAnalysesForFarmmock to be farm-specific.This mock currently returns both farms’ keys regardless of
b_id_farm, which can hide routing bugs.Proposed test-hardening change
- vi.spyOn(fdmCore, "getSoilAnalysesForFarm").mockResolvedValue( - new Map([ - ...mockFieldsData.map( - (f) => [f.b_id, mockSoilAnalysesData] as [string, typeof mockSoilAnalysesData], - ), - ...mockFieldsData2.map( - (f) => [f.b_id, mockSoilAnalysesData] as [string, typeof mockSoilAnalysesData], - ), - ]) as any, - ) + vi.spyOn(fdmCore, "getSoilAnalysesForFarm").mockImplementation( + async (_1, _2, b_id_farm) => + b_id_farm === "test-farm-id-2" + ? new Map(mockFieldsData2.map((f) => [f.b_id, mockSoilAnalysesData])) + : new Map(mockFieldsData.map((f) => [f.b_id, mockSoilAnalysesData])), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/balance/organic-matter/input.test.ts` around lines 397 - 406, The test currently has vi.spyOn(fdmCore, "getSoilAnalysesForFarm") returning a map containing keys for both farms (mockFieldsData and mockFieldsData2), which masks routing errors; change the mock to be farm-specific by replacing the static mockResolvedValue with a mockImplementation that inspects the incoming b_id_farm (or the function's farm identifier argument) and returns a Map built only from the matching field list (use mockFieldsData for farm A and mockFieldsData2 for farm B) each mapping to mockSoilAnalysesData; keep the return type compatible with the original (as any if needed) so tests exercise farm-specific behavior.fdm-core/src/cultivation.test.ts (1)
2538-2772: Add an unauthorized-principal test forgetCultivationsForFarm.This suite validates data semantics well; one explicit permission-denial test would complete the contract coverage for the new farm-scoped API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/cultivation.test.ts` around lines 2538 - 2772, Add a new test in the "getCultivationsForFarm" suite that verifies an unauthorized principal cannot access farm cultivations: create a second principal id (e.g., createId()), leave the farm owned by the original principal, then call getCultivationsForFarm(fdm, unauthorizedPrincipal, b_id_farm) and assert the call is rejected with an authorization/permission error (use the same rejection pattern your codebase uses — e.g., await expect(...).rejects.toThrow(/permission|unauthor/i/) or the specific UnauthorizedError class). Reference getCultivationsForFarm and createId to locate where to add the test and mirror the existing test style for async assertions.fdm-calculator/src/norms/nl/2026/value/input.ts (1)
124-148: Consider extracting shared soil-parameter picking logic.
a_p_cc/a_p_alextraction now exists in both single-field and farm-level paths. A small helper would prevent behavior drift between the two collectors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/norms/nl/2026/value/input.ts` around lines 124 - 148, Extract the repeated soil-parameter extraction into a small helper (e.g., pickSoilParameters or pickSoilAnalysisValues) that accepts the soil analysis array (soilAnalysis) and returns an object with a_p_cc and a_p_al, then replace the inline construction of soilAnalysisPicked in the loop (and the other single-field collector code path noted in the review) with a call to this helper; reference the existing symbols soilDataByField, soilAnalysisPicked, farmFields, and NL2026NormsInput when locating where to replace the logic so both collectors use the same implementation.fdm-core/src/soil.test.ts (1)
895-1093: Add an explicit unauthorized-principal test forgetSoilAnalysesForFarm.The new suite covers grouping/filtering well, but it doesn’t assert farm-level permission denial. Adding one negative case will lock in the intended auth contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/soil.test.ts` around lines 895 - 1093, Add a negative test that verifies getSoilAnalysesForFarm enforces farm-level permissions: create a second principal (use createId()), create a farm/field under the original principal with addFarm/addField as in beforeEach, then call getSoilAnalysesForFarm with the other principal and the original b_id_farm and assert the call rejects/throws an authorization error (e.g., expect(getSoilAnalysesForFarm(fdm, otherPrincipal, b_id_farm)).rejects.toThrow or equivalent) so the test ensures unauthorized principals cannot read farm soil analyses.fdm-core/src/cultivation.ts (1)
696-781: Extract the shared cultivation query/mapping.
getCultivationsForFarmnow duplicates the projection, ordering, and timeframe handling fromgetCultivations. A shared builder/mapper would reduce drift between the field-scoped and farm-scoped APIs.Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/cultivation.ts` around lines 696 - 781, getCultivationsForFarm duplicates the select/projection, joins, ordering and timeframe handling from getCultivations; extract that shared logic into a single helper (e.g., buildCultivationQuery or fetchCultivationRows) and a mapper (e.g., mapCultivationRowsToMap) so both getCultivationsForFarm and getCultivations call the same builder/mapper. Concretely: move the projection list, leftJoin/innerJoin sequence and orderBy into a reusable function that accepts (fdm, timeframeCondition, farmId or fieldId filter) and returns rows, and move the for-loop that groups rows by b_id into a mapper function; update getCultivationsForFarm to call buildCultivationTimeframeCondition, call the new shared query function, then call the shared mapper to produce the Map<string, Cultivation[]>, ensuring you reuse the existing symbols getCultivationsForFarm, getCultivations, buildCultivationTimeframeCondition, schema.cultivations*, and the grouping by b_id.fdm-core/src/harvest.ts (1)
342-363: Make the cultivation-keyed contract explicit in the API.Every other farm batch helper in this PR is field-keyed, so
Map<string, Harvest[]>here is easy to consume incorrectly. A named return type or a more explicit API name would make theb_lukeying much harder to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/harvest.ts` around lines 342 - 363, The function getHarvestsForFarm currently returns Promise<Map<string, Harvest[]>> keyed by cultivation ID (b_lu) which is inconsistent with other field-keyed helpers; make the cultivation-keying explicit by introducing a named return type or clearer API name: either define and return a CultivationHarvestMap (e.g., type alias or interface like CultivationHarvestMap = Map<CultivationId, Harvest[]>) and replace Promise<Map<string, Harvest[]>> with Promise<CultivationHarvestMap>, or rename the function to getHarvestsForFarmByCultivation and update its signature and JSDoc to state it returns a cultivation-keyed map (key = b_lu); update all call sites that consume getHarvestsForFarm accordingly to the new type/name and adjust JSDoc to mention b_lu is the key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/hand-old-cause.md:
- Line 13: The changelog entry for getCurrentSoilDataForFarm is ambiguous about
the map value shape; update the text to use the exact, single return value type
used in code (e.g., Map<b_id, CurrentSoilData[]> or Map<b_id, CurrentSoilData |
null>) instead of `Map<b_id, CurrentSoilData | []>` so it clearly matches the
function signature; locate the getCurrentSoilDataForFarm mention and replace the
ambiguous type with the canonical return type used in the codebase.
In `@fdm-calculator/src/norms/nl/2025/value/input.ts`:
- Around line 124-136: The fetched farmFields from getFields is not restricted
to the 2025 window and can include archived or future fields; update the
collector to only include fields valid in the 2025 timeframe by either calling
getFields with the timeframe2025 argument (if supported) or immediately
filtering the returned farmFields array against timeframe2025 (e.g., keep fields
whose validity intersects timeframe2025) before building
cultivationsByField/soilDataByField maps; modify the code around getFields and
the variables farmFields/timeframe2025 to ensure subsequent aggregation (in
getCultivationsForFarm/getCurrentSoilDataForFarm) only uses fields within the
2025 window.
In `@fdm-core/src/soil.ts`:
- Line 548: Fix the typo in the object mapping where a_clay_mi is being assigned
from the wrong property: change the assignment that currently uses row.clay_mi
to use row.a_clay_mi instead so the selected column a_clay_mi (from the query)
is actually returned; locate the mapping that sets a_clay_mi and update the
property reference accordingly.
---
Nitpick comments:
In `@fdm-calculator/src/balance/organic-matter/input.test.ts`:
- Around line 397-406: The test currently has vi.spyOn(fdmCore,
"getSoilAnalysesForFarm") returning a map containing keys for both farms
(mockFieldsData and mockFieldsData2), which masks routing errors; change the
mock to be farm-specific by replacing the static mockResolvedValue with a
mockImplementation that inspects the incoming b_id_farm (or the function's farm
identifier argument) and returns a Map built only from the matching field list
(use mockFieldsData for farm A and mockFieldsData2 for farm B) each mapping to
mockSoilAnalysesData; keep the return type compatible with the original (as any
if needed) so tests exercise farm-specific behavior.
In `@fdm-calculator/src/norms/nl/2026/value/input.ts`:
- Around line 124-148: Extract the repeated soil-parameter extraction into a
small helper (e.g., pickSoilParameters or pickSoilAnalysisValues) that accepts
the soil analysis array (soilAnalysis) and returns an object with a_p_cc and
a_p_al, then replace the inline construction of soilAnalysisPicked in the loop
(and the other single-field collector code path noted in the review) with a call
to this helper; reference the existing symbols soilDataByField,
soilAnalysisPicked, farmFields, and NL2026NormsInput when locating where to
replace the logic so both collectors use the same implementation.
In `@fdm-core/src/cultivation.test.ts`:
- Around line 2538-2772: Add a new test in the "getCultivationsForFarm" suite
that verifies an unauthorized principal cannot access farm cultivations: create
a second principal id (e.g., createId()), leave the farm owned by the original
principal, then call getCultivationsForFarm(fdm, unauthorizedPrincipal,
b_id_farm) and assert the call is rejected with an authorization/permission
error (use the same rejection pattern your codebase uses — e.g., await
expect(...).rejects.toThrow(/permission|unauthor/i/) or the specific
UnauthorizedError class). Reference getCultivationsForFarm and createId to
locate where to add the test and mirror the existing test style for async
assertions.
In `@fdm-core/src/cultivation.ts`:
- Around line 696-781: getCultivationsForFarm duplicates the select/projection,
joins, ordering and timeframe handling from getCultivations; extract that shared
logic into a single helper (e.g., buildCultivationQuery or fetchCultivationRows)
and a mapper (e.g., mapCultivationRowsToMap) so both getCultivationsForFarm and
getCultivations call the same builder/mapper. Concretely: move the projection
list, leftJoin/innerJoin sequence and orderBy into a reusable function that
accepts (fdm, timeframeCondition, farmId or fieldId filter) and returns rows,
and move the for-loop that groups rows by b_id into a mapper function; update
getCultivationsForFarm to call buildCultivationTimeframeCondition, call the new
shared query function, then call the shared mapper to produce the Map<string,
Cultivation[]>, ensuring you reuse the existing symbols getCultivationsForFarm,
getCultivations, buildCultivationTimeframeCondition, schema.cultivations*, and
the grouping by b_id.
In `@fdm-core/src/fertilizer.test.ts`:
- Around line 1411-1691: Add a negative auth test for
getFertilizerApplicationsForFarm: create a second principal (e.g.,
principal_no_access) who does NOT own or have permissions on b_id_farm, then
call getFertilizerApplicationsForFarm(fdm, principal_no_access, b_id_farm) and
assert it fails with the expected permission-denied behavior (rejects/throws or
returns an auth error) instead of returning data; place this case alongside the
other tests in the describe block and mirror setup using existing helpers
(createId, addFarm, addField, addFertilizerApplication) so the test verifies
farm-level authorization is enforced.
In `@fdm-core/src/harvest.ts`:
- Around line 342-363: The function getHarvestsForFarm currently returns
Promise<Map<string, Harvest[]>> keyed by cultivation ID (b_lu) which is
inconsistent with other field-keyed helpers; make the cultivation-keying
explicit by introducing a named return type or clearer API name: either define
and return a CultivationHarvestMap (e.g., type alias or interface like
CultivationHarvestMap = Map<CultivationId, Harvest[]>) and replace
Promise<Map<string, Harvest[]>> with Promise<CultivationHarvestMap>, or rename
the function to getHarvestsForFarmByCultivation and update its signature and
JSDoc to state it returns a cultivation-keyed map (key = b_lu); update all call
sites that consume getHarvestsForFarm accordingly to the new type/name and
adjust JSDoc to mention b_lu is the key.
In `@fdm-core/src/soil.test.ts`:
- Around line 895-1093: Add a negative test that verifies getSoilAnalysesForFarm
enforces farm-level permissions: create a second principal (use createId()),
create a farm/field under the original principal with addFarm/addField as in
beforeEach, then call getSoilAnalysesForFarm with the other principal and the
original b_id_farm and assert the call rejects/throws an authorization error
(e.g., expect(getSoilAnalysesForFarm(fdm, otherPrincipal,
b_id_farm)).rejects.toThrow or equivalent) so the test ensures unauthorized
principals cannot read farm soil analyses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b3c05c70-565c-4324-ab18-c60fb10fa4b5
📒 Files selected for processing (21)
.changeset/fast-farms-fetch.md.changeset/hand-old-cause.mdfdm-calculator/src/balance/nitrogen/input.test.tsfdm-calculator/src/balance/nitrogen/input.tsfdm-calculator/src/balance/organic-matter/input.test.tsfdm-calculator/src/balance/organic-matter/input.tsfdm-calculator/src/norms/index.tsfdm-calculator/src/norms/nl/2025/filling/input.tsfdm-calculator/src/norms/nl/2025/value/input.tsfdm-calculator/src/norms/nl/2026/filling/input.tsfdm-calculator/src/norms/nl/2026/value/input.tsfdm-core/src/cultivation.test.tsfdm-core/src/cultivation.tsfdm-core/src/fertilizer.test.tsfdm-core/src/fertilizer.tsfdm-core/src/harvest.test.tsfdm-core/src/harvest.tsfdm-core/src/index.tsfdm-core/src/soil.d.tsfdm-core/src/soil.test.tsfdm-core/src/soil.ts
Collaborator
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Collaborator
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
BoraIneviNMI
approved these changes
Apr 2, 2026
Collaborator
There was a problem hiding this comment.
Seems to work fine. The test coverage is good too.
We can merge this and close #530
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
fdm-calculator/src/norms/nl/2025/filling/input.test.ts (1)
207-229: Consider strengthening test assertions and call verifications.The test validates core functionality but could be more thorough:
Missing property assertions: The single-field test checks
b_centroidandfertilizers, but this farm-level test doesn't verify these properties in the result.Incomplete call argument verification: Lines 227-228 only check that the functions were called, but don't verify the arguments (unlike line 226 for
getFields). This could miss regressions where incorrect parameters are passed.💡 Suggested improvements
expect(fieldInput.cultivations).toHaveLength(1) expect(fieldInput.applications).toHaveLength(1) + expect(fieldInput.b_centroid).toEqual([10, 20]) + expect(fieldInput.fertilizers).toHaveLength(1) expect(getFields).toHaveBeenCalledWith(mockFdm, mockPrincipalId, mockFarmId) - expect(getCultivationsForFarm).toHaveBeenCalled() - expect(getFertilizerApplicationsForFarm).toHaveBeenCalled() + expect(getCultivationsForFarm).toHaveBeenCalledWith( + mockFdm, + mockPrincipalId, + mockFarmId, + expect.objectContaining({ start: expect.any(Date), end: expect.any(Date) }), + ) + expect(getFertilizerApplicationsForFarm).toHaveBeenCalledWith( + mockFdm, + mockPrincipalId, + mockFarmId, + expect.objectContaining({ start: expect.any(Date), end: expect.any(Date) }), + )Also consider adding edge case tests similar to the comments in lines 173-179 (e.g., empty fields, field without
fosfaatgebruiksnormentry).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/norms/nl/2025/filling/input.test.ts` around lines 207 - 229, The farm-level test for collectNL2025InputForFertilizerApplicationFillingForFarm is missing assertions for per-field properties and incomplete call-argument checks: add assertions on the retrieved fieldInput.b_centroid and fieldInput.fertilizers (same checks as the single-field test) and verify that getCultivationsForFarm and getFertilizerApplicationsForFarm were called with the expected arguments (mockFdm, mockPrincipalId, mockFarmId) similar to the existing getFields assertion; optionally add additional edge-case tests (empty fields, field without fosfaatgebruiksnorm) mirroring the single-field cases to cover regressions.fdm-calculator/src/norms/nl/2026/filling/input.test.ts (2)
179-227: Add edge-case farm tests (multi-field and empty maps).Please add coverage for at least:
- multiple fields with independent
fosfaatgebruiksnormvalues, and- empty results from
getCultivationsForFarm/getFertilizerApplicationsForFarm(should yield stable per-field defaults).This will better validate grouping behavior required by the farm-batch collector design.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/norms/nl/2026/filling/input.test.ts` around lines 179 - 227, Add two new tests in input.test.ts that call collectNL2026InputForFertilizerApplicationFillingForFarm: one where getFields returns multiple Field objects and fosfaatgebruiksnormByField is a Map with independent values for each field (assert the result Map contains both fields with the correct fosfaatgebruiksnorm and other flags), and one where getCultivationsForFarm and getFertilizerApplicationsForFarm are mocked to return empty Maps (or Maps missing entries) to assert per-field defaults: empty cultivations/applications arrays, stable has_grazing_intention and has_organic_certification behavior, and that getFields/getCultivationsForFarm/getFertilizerApplicationsForFarm were called; keep using the existing mocked helpers (getFields, getCultivationsForFarm, getFertilizerApplicationsForFarm, getGrazingIntention, isOrganicCertificationValid) and reference collectNL2026InputForFertilizerApplicationFillingForFarm in the assertions.
223-225: Strengthen batch-call assertions to prevent N+1 regressions.
toHaveBeenCalled()is too loose here. Assert full arguments/timeframe, and explicitly verify field-level APIs are not used in the farm collector path.Proposed assertion hardening
- expect(getCultivationsForFarm).toHaveBeenCalled() - expect(getFertilizerApplicationsForFarm).toHaveBeenCalled() + expect(getCultivationsForFarm).toHaveBeenCalledWith( + mockFdm, + mockPrincipalId, + mockFarmId, + { + start: new Date(2026, 0, 1), + end: new Date(2026, 11, 31, 23, 59, 59, 999), + }, + ) + expect(getFertilizerApplicationsForFarm).toHaveBeenCalledWith( + mockFdm, + mockPrincipalId, + mockFarmId, + { + start: new Date(2026, 0, 1), + end: new Date(2026, 11, 31, 23, 59, 59, 999), + }, + ) + expect(getField).not.toHaveBeenCalled() + expect(getCultivations).not.toHaveBeenCalled() + expect(getFertilizerApplications).not.toHaveBeenCalled()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/norms/nl/2026/filling/input.test.ts` around lines 223 - 225, The assertions are too loose: replace the toHaveBeenCalled() checks for getCultivationsForFarm and getFertilizerApplicationsForFarm with precise toHaveBeenCalledWith(...) assertions including the expected farm id/timeframe (use mockFarmId and the same timeframe values passed to the collector in the test) and assert getFields and any field-level APIs (e.g., getCultivationsForField, getFertilizerApplicationsForField) are NOT called to prevent N+1 regressions; keep the existing getFields assertion only if the test path legitimately expects it (asserting exact args: mockFdm, mockPrincipalId, mockFarmId) otherwise change it to expect not.toHaveBeenCalled().fdm-core/src/harvest.test.ts (1)
1011-1019: Consider adding an authorization test.The PR objectives mention adding tests for authorization of the batch functions. Consider adding a test case to verify that an unauthorized principal cannot retrieve harvests for a farm they don't have access to, similar to the existing permission tests in the
Harvest Data Modelsuite.Example test case
it("should throw an error when retrieving harvests without permission", async () => { await addHarvest(fdm, principal_id, b_lu, new Date("2024-08-01"), { b_lu_yield_fresh: 5000, b_lu_moist: 15, b_lu_cp: 110, }) const other_principal_id = createId() await expect( getHarvestsForFarm(fdm, other_principal_id, b_id_farm) ).rejects.toThrowError( "Principal does not have permission to perform this action" ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/harvest.test.ts` around lines 1011 - 1019, Add an authorization unit test to the "getHarvestsForFarm" suite that verifies unauthorized principals cannot retrieve farm harvests: create a harvest using addHarvest(fdm, principal_id, b_lu, ...), then generate a different principal id (e.g., createId()) and call getHarvestsForFarm(fdm, other_principal_id, b_id_farm) and assert it rejects with the permission error message (match "Principal does not have permission to perform this action"); ensure the test uses the existing FdmServerType test setup and the same farm/landuse identifiers (principal_id, b_id_farm, b_lu) so it mirrors the other permission tests in the Harvest Data Model suite.fdm-core/src/soil.ts (1)
472-589: Extract the shared soil-analysis projection.This select + mapper now duplicates the field-scoped
getSoilAnalyses(...)path almost verbatim. Pulling the projection and row-to-SoilAnalysisconversion into one helper will make the farm and field variants evolve together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/soil.ts` around lines 472 - 589, Duplicate selection projection and row-to-SoilAnalysis mapping should be extracted into a shared helper; create a function that returns the projection object used in the fdm.select(...) (e.g., soilAnalysisProjection or buildSoilAnalysisProjection) and another helper (e.g., mapRowToSoilAnalysis(row): SoilAnalysis) that converts a DB row to the SoilAnalysis shape, then use these helpers in this query and the field-scoped getSoilAnalyses(...) path to replace the long inline select({...}) and the manual construction of the soilAnalysis object so both variants reuse the same projection and mapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-calculator/src/norms/nl/2025/value/input.test.ts`:
- Around line 107-150: The test currently verifies the assembled Map but not
that collectNL2025InputForNormsForFarm actually stayed on the farm-scoped path
and used the 2024/2025 batch-helper contract; add assertions after calling
collectNL2025InputForNormsForFarm to verify that fdmCore methods used for
farm-scoped collection were invoked with the farm id and the expected 2024/2025
window/contract: e.g., assert vi.mocked(fdmCore.getFields) was called with
mockFarmId (or farm-scoped args), and assert
vi.mocked(fdmCore.getCultivationsForFarm) and
vi.mocked(fdmCore.getCurrentSoilDataForFarm) were called with mockFarmId and the
expected 2024/2025 window/contract identifier (or year range) so the test ties
the collector to the NL-2025 batch-helper path.
In `@fdm-calculator/src/norms/nl/2026/value/input.test.ts`:
- Around line 98-139: After calling collectNL2026InputForNormsForFarm, add
assertions that the batch/farm-scoped helpers were used exactly once and the
per-field helpers were not used: assert fdmCore.getCultivationsForFarm and
fdmCore.getCurrentSoilDataForFarm were called (and called once) with the farm id
(mockFarmId) and that fdmCore.getCultivationsForField and
fdmCore.getCurrentSoilDataForField were not called; this proves the batch path
is exercised rather than falling back to per-field helpers.
In `@fdm-core/src/harvest.test.ts`:
- Around line 1263-1303: The two tests "should filter harvests when only
timeframe start is provided" and "should filter harvests when only timeframe end
is provided" currently pass full timeframe objects; change them to pass partial
timeframe objects to exercise getHarvestsForFarm's partial-timeframe logic: in
the start-only test set timeframe = { start: new Date("2024-01-01") } (omit end)
and in the end-only test set timeframe = { end: new Date("2024-12-31") } (omit
start), leaving the addHarvest call and assertions unchanged so addHarvest(...)
and getHarvestsForFarm(fdm, principal_id, b_id_farm, timeframe) are called with
the partial timeframe values.
---
Nitpick comments:
In `@fdm-calculator/src/norms/nl/2025/filling/input.test.ts`:
- Around line 207-229: The farm-level test for
collectNL2025InputForFertilizerApplicationFillingForFarm is missing assertions
for per-field properties and incomplete call-argument checks: add assertions on
the retrieved fieldInput.b_centroid and fieldInput.fertilizers (same checks as
the single-field test) and verify that getCultivationsForFarm and
getFertilizerApplicationsForFarm were called with the expected arguments
(mockFdm, mockPrincipalId, mockFarmId) similar to the existing getFields
assertion; optionally add additional edge-case tests (empty fields, field
without fosfaatgebruiksnorm) mirroring the single-field cases to cover
regressions.
In `@fdm-calculator/src/norms/nl/2026/filling/input.test.ts`:
- Around line 179-227: Add two new tests in input.test.ts that call
collectNL2026InputForFertilizerApplicationFillingForFarm: one where getFields
returns multiple Field objects and fosfaatgebruiksnormByField is a Map with
independent values for each field (assert the result Map contains both fields
with the correct fosfaatgebruiksnorm and other flags), and one where
getCultivationsForFarm and getFertilizerApplicationsForFarm are mocked to return
empty Maps (or Maps missing entries) to assert per-field defaults: empty
cultivations/applications arrays, stable has_grazing_intention and
has_organic_certification behavior, and that
getFields/getCultivationsForFarm/getFertilizerApplicationsForFarm were called;
keep using the existing mocked helpers (getFields, getCultivationsForFarm,
getFertilizerApplicationsForFarm, getGrazingIntention,
isOrganicCertificationValid) and reference
collectNL2026InputForFertilizerApplicationFillingForFarm in the assertions.
- Around line 223-225: The assertions are too loose: replace the
toHaveBeenCalled() checks for getCultivationsForFarm and
getFertilizerApplicationsForFarm with precise toHaveBeenCalledWith(...)
assertions including the expected farm id/timeframe (use mockFarmId and the same
timeframe values passed to the collector in the test) and assert getFields and
any field-level APIs (e.g., getCultivationsForField,
getFertilizerApplicationsForField) are NOT called to prevent N+1 regressions;
keep the existing getFields assertion only if the test path legitimately expects
it (asserting exact args: mockFdm, mockPrincipalId, mockFarmId) otherwise change
it to expect not.toHaveBeenCalled().
In `@fdm-core/src/harvest.test.ts`:
- Around line 1011-1019: Add an authorization unit test to the
"getHarvestsForFarm" suite that verifies unauthorized principals cannot retrieve
farm harvests: create a harvest using addHarvest(fdm, principal_id, b_lu, ...),
then generate a different principal id (e.g., createId()) and call
getHarvestsForFarm(fdm, other_principal_id, b_id_farm) and assert it rejects
with the permission error message (match "Principal does not have permission to
perform this action"); ensure the test uses the existing FdmServerType test
setup and the same farm/landuse identifiers (principal_id, b_id_farm, b_lu) so
it mirrors the other permission tests in the Harvest Data Model suite.
In `@fdm-core/src/soil.ts`:
- Around line 472-589: Duplicate selection projection and row-to-SoilAnalysis
mapping should be extracted into a shared helper; create a function that returns
the projection object used in the fdm.select(...) (e.g., soilAnalysisProjection
or buildSoilAnalysisProjection) and another helper (e.g.,
mapRowToSoilAnalysis(row): SoilAnalysis) that converts a DB row to the
SoilAnalysis shape, then use these helpers in this query and the field-scoped
getSoilAnalyses(...) path to replace the long inline select({...}) and the
manual construction of the soilAnalysis object so both variants reuse the same
projection and mapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 98b6dfec-460c-4aed-851e-2b959f885999
📒 Files selected for processing (14)
.changeset/hand-old-cause.mdfdm-calculator/src/balance/nitrogen/input.test.tsfdm-calculator/src/balance/organic-matter/input.test.tsfdm-calculator/src/norms/nl/2025/filling/input.test.tsfdm-calculator/src/norms/nl/2025/value/input.test.tsfdm-calculator/src/norms/nl/2025/value/input.tsfdm-calculator/src/norms/nl/2026/filling/input.test.tsfdm-calculator/src/norms/nl/2026/value/input.test.tsfdm-calculator/src/norms/nl/2026/value/input.tsfdm-core/src/cultivation.test.tsfdm-core/src/fertilizer.test.tsfdm-core/src/harvest.test.tsfdm-core/src/soil.test.tsfdm-core/src/soil.ts
✅ Files skipped from review due to trivial changes (3)
- .changeset/hand-old-cause.md
- fdm-core/src/fertilizer.test.ts
- fdm-core/src/cultivation.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- fdm-calculator/src/norms/nl/2025/value/input.ts
- fdm-core/src/soil.test.ts
- fdm-calculator/src/balance/nitrogen/input.test.ts
- fdm-calculator/src/balance/organic-matter/input.test.ts
Comment on lines
+1263
to
+1303
| it("should filter harvests when only timeframe start is provided", async () => { | ||
| await addHarvest(fdm, principal_id, b_lu, new Date("2024-08-01"), { | ||
| b_lu_yield_fresh: 5000, | ||
| b_lu_moist: 15, | ||
| b_lu_cp: 110, | ||
| }) | ||
|
|
||
| const timeframe = { | ||
| start: new Date("2024-01-01"), | ||
| end: new Date("2024-12-31"), | ||
| } | ||
| const result = await getHarvestsForFarm( | ||
| fdm, | ||
| principal_id, | ||
| b_id_farm, | ||
| timeframe, | ||
| ) | ||
|
|
||
| expect(result.has(b_lu)).toBe(true) | ||
| }) | ||
|
|
||
| it("should filter harvests when only timeframe end is provided", async () => { | ||
| await addHarvest(fdm, principal_id, b_lu, new Date("2024-08-01"), { | ||
| b_lu_yield_fresh: 5000, | ||
| b_lu_moist: 15, | ||
| b_lu_cp: 110, | ||
| }) | ||
|
|
||
| const timeframe = { | ||
| start: new Date("2024-01-01"), | ||
| end: new Date("2024-12-31"), | ||
| } | ||
| const result = await getHarvestsForFarm( | ||
| fdm, | ||
| principal_id, | ||
| b_id_farm, | ||
| timeframe, | ||
| ) | ||
|
|
||
| expect(result.has(b_lu)).toBe(true) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the signature of getHarvestsForFarm to understand timeframe parameter type
ast-grep --pattern $'export async function getHarvestsForFarm($$$) {
$$$
}'
# Also check if timeframe has optional start/end in type definition
rg -n "timeframe.*start|timeframe.*end" --type=ts -C3Repository: nmi-agro/fdm
Length of output: 50368
Test implementations don't match their names—provide partial timeframes as the test titles claim.
Both tests claim to verify partial timeframe filtering (start-only and end-only), but both provide complete timeframe objects with start and end. Since getHarvestsForFarm supports partial timeframes (as shown by its conditional logic), these tests should actually test that behavior. Update them:
Proposed fix
it("should filter harvests when only timeframe start is provided", async () => {
await addHarvest(fdm, principal_id, b_lu, new Date("2024-08-01"), {
b_lu_yield_fresh: 5000,
b_lu_moist: 15,
b_lu_cp: 110,
})
const timeframe = {
start: new Date("2024-01-01"),
- end: new Date("2024-12-31"),
}
const result = await getHarvestsForFarm(
fdm,
principal_id,
b_id_farm,
timeframe,
)
expect(result.has(b_lu)).toBe(true)
})
it("should filter harvests when only timeframe end is provided", async () => {
await addHarvest(fdm, principal_id, b_lu, new Date("2024-08-01"), {
b_lu_yield_fresh: 5000,
b_lu_moist: 15,
b_lu_cp: 110,
})
const timeframe = {
- start: new Date("2024-01-01"),
end: new Date("2024-12-31"),
}
const result = await getHarvestsForFarm(
fdm,
principal_id,
b_id_farm,
timeframe,
)
expect(result.has(b_lu)).toBe(true)
})📝 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.
Suggested change
| it("should filter harvests when only timeframe start is provided", async () => { | |
| await addHarvest(fdm, principal_id, b_lu, new Date("2024-08-01"), { | |
| b_lu_yield_fresh: 5000, | |
| b_lu_moist: 15, | |
| b_lu_cp: 110, | |
| }) | |
| const timeframe = { | |
| start: new Date("2024-01-01"), | |
| end: new Date("2024-12-31"), | |
| } | |
| const result = await getHarvestsForFarm( | |
| fdm, | |
| principal_id, | |
| b_id_farm, | |
| timeframe, | |
| ) | |
| expect(result.has(b_lu)).toBe(true) | |
| }) | |
| it("should filter harvests when only timeframe end is provided", async () => { | |
| await addHarvest(fdm, principal_id, b_lu, new Date("2024-08-01"), { | |
| b_lu_yield_fresh: 5000, | |
| b_lu_moist: 15, | |
| b_lu_cp: 110, | |
| }) | |
| const timeframe = { | |
| start: new Date("2024-01-01"), | |
| end: new Date("2024-12-31"), | |
| } | |
| const result = await getHarvestsForFarm( | |
| fdm, | |
| principal_id, | |
| b_id_farm, | |
| timeframe, | |
| ) | |
| expect(result.has(b_lu)).toBe(true) | |
| }) | |
| it("should filter harvests when only timeframe start is provided", async () => { | |
| await addHarvest(fdm, principal_id, b_lu, new Date("2024-08-01"), { | |
| b_lu_yield_fresh: 5000, | |
| b_lu_moist: 15, | |
| b_lu_cp: 110, | |
| }) | |
| const timeframe = { | |
| start: new Date("2024-01-01"), | |
| } | |
| const result = await getHarvestsForFarm( | |
| fdm, | |
| principal_id, | |
| b_id_farm, | |
| timeframe, | |
| ) | |
| expect(result.has(b_lu)).toBe(true) | |
| }) | |
| it("should filter harvests when only timeframe end is provided", async () => { | |
| await addHarvest(fdm, principal_id, b_lu, new Date("2024-08-01"), { | |
| b_lu_yield_fresh: 5000, | |
| b_lu_moist: 15, | |
| b_lu_cp: 110, | |
| }) | |
| const timeframe = { | |
| end: new Date("2024-12-31"), | |
| } | |
| const result = await getHarvestsForFarm( | |
| fdm, | |
| principal_id, | |
| b_id_farm, | |
| timeframe, | |
| ) | |
| expect(result.has(b_lu)).toBe(true) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-core/src/harvest.test.ts` around lines 1263 - 1303, The two tests "should
filter harvests when only timeframe start is provided" and "should filter
harvests when only timeframe end is provided" currently pass full timeframe
objects; change them to pass partial timeframe objects to exercise
getHarvestsForFarm's partial-timeframe logic: in the start-only test set
timeframe = { start: new Date("2024-01-01") } (omit end) and in the end-only
test set timeframe = { end: new Date("2024-12-31") } (omit start), leaving the
addHarvest call and assertions unchanged so addHarvest(...) and
getHarvestsForFarm(fdm, principal_id, b_id_farm, timeframe) are called with the
partial timeframe values.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
fdm-calculator/src/norms/nl/2026/filling/input.test.ts (1)
179-229: Test suite covers the key farm-level scenarios well.The test correctly verifies:
- Farm-scoped boolean flags (
has_grazing_intention,has_organic_certification)- Per-field
fosfaatgebruiksnormfrom the input map- Batch API calls with the NL 2026 timeframe
For additional completeness, consider asserting that the single-field APIs are NOT called (similar to the 2025 tests):
🧪 Optional assertion additions
expect(getFields).toHaveBeenCalledWith(mockFdm, mockPrincipalId, mockFarmId) expect(getCultivationsForFarm).toHaveBeenCalledWith(mockFdm, mockPrincipalId, mockFarmId, timeframe2026) expect(getFertilizerApplicationsForFarm).toHaveBeenCalledWith(mockFdm, mockPrincipalId, mockFarmId, timeframe2026) + expect(getField).not.toHaveBeenCalled() + expect(getCultivations).not.toHaveBeenCalled() + expect(getFertilizerApplications).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/norms/nl/2026/filling/input.test.ts` around lines 179 - 229, Add assertions to ensure single-field API helpers are not used in the farm-level test: after the existing timeframe assertions, add negative checks like expect(getField).not.toHaveBeenCalled(), expect(getCultivationsForField).not.toHaveBeenCalled(), and expect(getFertilizerApplicationsForField).not.toHaveBeenCalled() so the test verifies we used the batch helpers (getFields, getCultivationsForFarm, getFertilizerApplicationsForFarm) instead of per-field calls.fdm-calculator/src/norms/nl/2025/value/input.test.ts (1)
107-158: Test suite for farm-level collector looks solid; consider addinggetFieldassertion for completeness.The test correctly:
- Verifies the returned
Mapstructure and per-field input assembly- Asserts farm-scoped APIs are called with the NL 2025 timeframes
- Asserts the single-field APIs (
getCultivations,getCurrentSoilData) are NOT calledFor full coverage of the farm-scoped control flow, consider also verifying that the single-field
getFieldis not called:🧪 Optional assertion addition
expect(fdmCore.getCurrentSoilDataForFarm).toHaveBeenCalledWith(mockFdm, mockPrincipalId, mockFarmId, timeframe2025) + expect(fdmCore.getField).not.toHaveBeenCalled() expect(fdmCore.getCultivations).not.toHaveBeenCalled() expect(fdmCore.getCurrentSoilData).not.toHaveBeenCalled()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/norms/nl/2025/value/input.test.ts` around lines 107 - 158, Add an assertion that the single-field API fdmCore.getField is not called to fully cover farm-scoped control flow: in the test for collectNL2025InputForNormsForFarm, after the existing expects about getFields/getCultivations/getCurrentSoilData, add expect(fdmCore.getField).not.toHaveBeenCalled() to ensure only farm-scoped (fdmCore.getFields, fdmCore.getCultivationsForFarm, fdmCore.getCurrentSoilDataForFarm) functions are used and single-field helpers like getField/getCultivations/getCurrentSoilData remain unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fdm-calculator/src/norms/nl/2025/value/input.test.ts`:
- Around line 107-158: Add an assertion that the single-field API
fdmCore.getField is not called to fully cover farm-scoped control flow: in the
test for collectNL2025InputForNormsForFarm, after the existing expects about
getFields/getCultivations/getCurrentSoilData, add
expect(fdmCore.getField).not.toHaveBeenCalled() to ensure only farm-scoped
(fdmCore.getFields, fdmCore.getCultivationsForFarm,
fdmCore.getCurrentSoilDataForFarm) functions are used and single-field helpers
like getField/getCultivations/getCurrentSoilData remain unused.
In `@fdm-calculator/src/norms/nl/2026/filling/input.test.ts`:
- Around line 179-229: Add assertions to ensure single-field API helpers are not
used in the farm-level test: after the existing timeframe assertions, add
negative checks like expect(getField).not.toHaveBeenCalled(),
expect(getCultivationsForField).not.toHaveBeenCalled(), and
expect(getFertilizerApplicationsForField).not.toHaveBeenCalled() so the test
verifies we used the batch helpers (getFields, getCultivationsForFarm,
getFertilizerApplicationsForFarm) instead of per-field calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 90117b62-72bc-4b69-b920-723be3b2ad19
📒 Files selected for processing (6)
fdm-calculator/src/norms/nl/2025/filling/input.test.tsfdm-calculator/src/norms/nl/2025/value/input.test.tsfdm-calculator/src/norms/nl/2026/filling/input.test.tsfdm-calculator/src/norms/nl/2026/value/input.test.tsfdm-core/src/harvest.test.tsfdm-core/vitest.config.ts
✅ Files skipped from review due to trivial changes (1)
- fdm-core/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- fdm-calculator/src/norms/nl/2026/value/input.test.ts
- fdm-calculator/src/norms/nl/2025/filling/input.test.ts
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Closes #518