Enhance Fertilizer System: Catalog, UI, and Test Updates#111
Conversation
…ers for that specific farm
🦋 Changeset detectedLatest commit: 5a3d4b5 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 |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request introduces multiple enhancements across the fertilizer management system. It updates several packages with minor version bumps, adds new functions for adding and updating fertilizers in the catalogue (including custom fertilizers), and modifies data retrieval and hashing logic. UI components and forms have been extended to support fertilizer selection and editing, while routes and loaders now handle new fertilizer flows. Additionally, the test suites for catalogue synchronization and fertilizer operations have been refined, and a new dependency for advanced table functionality has been added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as FertilizerForm
participant Route as FertilizerAction
participant Service as addFertilizerToCatalogue
participant DB
User->>UI: Fill and submit new fertilizer data
UI->>Route: POST form data
Route->>Service: Invoke addFertilizerToCatalogue(principal_id, b_id_farm, properties)
Service->>Service: Compute hash (via hashFertilizer)
Service->>DB: Insert new fertilizer record
DB-->>Service: Return new fertilizer ID
Service-->>Route: Forward fertilizer ID
Route-->>UI: Send success response
UI->>User: Display updated catalogue
sequenceDiagram
participant User
participant UI as FertilizerUpdateForm
participant Route as UpdateAction
participant Service as updateFertilizerFromCatalogue
participant DB
User->>UI: Submit updated fertilizer details
UI->>Route: POST updated data
Route->>Service: Invoke updateFertilizerFromCatalogue(principal_id, b_id_farm, p_id_catalogue, updatedProperties)
Service->>Service: Generate new hash (via hashFertilizer)
Service->>DB: Update fertilizer record
DB-->>Service: Confirm update
Service-->>Route: Return success status
Route-->>UI: Deliver updated info
UI->>User: Show updated fertilizer details
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #111 +/- ##
===============================================
+ Coverage 91.83% 92.06% +0.23%
===============================================
Files 40 43 +3
Lines 4299 4475 +176
Branches 422 440 +18
===============================================
+ Hits 3948 4120 +172
- Misses 350 354 +4
Partials 1 1
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:
|
…ustom fertilizer
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <37927107+SvenVw@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fdm-core/src/fertilizer.ts (1)
453-458: Add farm ID to error context for better debugging.The error context should include the farm ID to make debugging easier in case of failures.
throw handleError(err, "Exception for updateFertilizerFromCatalogue", { p_id_catalogue, + b_id_farm, properties, })🧰 Tools
🪛 GitHub Actions: Tests
[warning] 453-453: @rollup/plugin-typescript TS1128: Declaration or statement expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fdm-core/src/fertilizer.ts(9 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
fdm-core/src/fertilizer.ts (4)
fdm-core/src/authorization.d.ts (1)
PrincipalId(21-23)fdm-core/src/db/schema.ts (2)
farmsTypeInsert(32-32)fertilizersCatalogueTypeInsert(239-240)fdm-core/src/authorization.ts (1)
checkPermission(155-239)fdm-core/src/error.ts (1)
handleError(15-31)
🪛 Biome (1.9.4)
fdm-core/src/fertilizer.ts
[error] 399-400: Expected a property, or a signature but instead found '-'.
Expected a property, or a signature here.
(parse)
[error] 400-400: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 411-459: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🪛 GitHub Actions: Tests
fdm-core/src/fertilizer.ts
[warning] 400-400: @rollup/plugin-typescript TS1131: Property or signature expected.
[warning] 400-400: @rollup/plugin-typescript TS1005: ';' expected.
[warning] 401-401: @rollup/plugin-typescript TS1005: ';' expected.
[warning] 405-405: @rollup/plugin-typescript TS1005: ';' expected.
[warning] 406-406: @rollup/plugin-typescript TS1005: ';' expected.
[warning] 410-410: @rollup/plugin-typescript TS1128: Declaration or statement expected.
[warning] 410-410: @rollup/plugin-typescript TS1109: Expression expected.
[warning] 411-411: @rollup/plugin-typescript TS1109: Expression expected.
[warning] 412-412: @rollup/plugin-typescript TS1005: ':' expected.
[warning] 413-413: @rollup/plugin-typescript TS1005: ',' expected.
[warning] 415-415: @rollup/plugin-typescript TS1003: Identifier expected.
[warning] 415-415: @rollup/plugin-typescript TS1138: Parameter declaration expected.
[warning] 416-416: @rollup/plugin-typescript TS1003: Identifier expected.
[warning] 416-416: @rollup/plugin-typescript TS1138: Parameter declaration expected.
[warning] 419-419: @rollup/plugin-typescript TS1003: Identifier expected.
[warning] 419-419: @rollup/plugin-typescript TS1138: Parameter declaration expected.
[warning] 422-422: @rollup/plugin-typescript TS1005: ',' expected.
[warning] 434-434: @rollup/plugin-typescript TS1005: ',' expected.
[warning] 434-434: @rollup/plugin-typescript TS1005: ',' expected.
[warning] 434-434: @rollup/plugin-typescript TS1136: Property assignment expected.
[warning] 453-453: @rollup/plugin-typescript TS1128: Declaration or statement expected.
[warning] 459-459: @rollup/plugin-typescript TS1128: Declaration or statement expected.
[error] 228-228: RollupError: Expression expected (Note that you need plugins to import files that are not JavaScript)
🔇 Additional comments (6)
fdm-core/src/fertilizer.ts (6)
1-1: Import changes look good.Added imports for
andfrom drizzle-orm andhashFertilizerfrom @svenvw/fdm-data match the functional changes in the code.Also applies to: 13-13
75-85: Function signature updates are appropriate.The updated JSDoc comments and function signature now properly include the
principal_idandb_id_farmparameters, which align with the implementation changes that follow.Also applies to: 86-90
138-161: Implementation of addFertilizerToCatalogue looks good.The function now:
- Correctly performs permission checks
- Returns the ID of the added fertilizer
- Uses the
hashFertilizerfunction to generate a hash for the fertilizer properties- Sets the farm ID as the source
These changes improve security and traceability of fertilizer catalog entries.
264-266: New fields added to getFertilizer and getFertilizers.Adding fields like
p_id_catalogue,p_source,p_dm,p_density, and type information (p_type_manure, etc.) to the query results enables more detailed fertilizer information in the UI and client applications.Also applies to: 273-285, 313-315, 493-495, 502-514, 542-544
399-401: Fixed parameter naming inconsistencies.The naming inconsistencies in the properties list have been correctly addressed:
pr_cr_rt→p_cr_rtp_cl_rtreferencingp_cl_cr→p_cl_rtreferencingp_cl_rtThis fixes the syntax errors flagged by the static analysis tools.
Also applies to: 405-406
🧰 Tools
🪛 Biome (1.9.4)
[error] 399-400: Expected a property, or a signature but instead found '-'.
Expected a property, or a signature here.
(parse)
[error] 400-400: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
🪛 GitHub Actions: Tests
[warning] 400-400: @rollup/plugin-typescript TS1131: Property or signature expected.
[warning] 400-400: @rollup/plugin-typescript TS1005: ';' expected.
[warning] 401-401: @rollup/plugin-typescript TS1005: ';' expected.
344-459: New updateFertilizerFromCatalogue function looks good.The function properly implements the update operation with appropriate permission checks, existence validation, and hash recalculation. The
andcondition in thewhereclause for both the select and update operations correctly ensures that users can only update fertilizers associated with their farm.🧰 Tools
🪛 Biome (1.9.4)
[error] 399-400: Expected a property, or a signature but instead found '-'.
Expected a property, or a signature here.
(parse)
[error] 400-400: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 411-459: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🪛 GitHub Actions: Tests
[warning] 400-400: @rollup/plugin-typescript TS1131: Property or signature expected.
[warning] 400-400: @rollup/plugin-typescript TS1005: ';' expected.
[warning] 401-401: @rollup/plugin-typescript TS1005: ';' expected.
[warning] 405-405: @rollup/plugin-typescript TS1005: ';' expected.
[warning] 406-406: @rollup/plugin-typescript TS1005: ';' expected.
[warning] 410-410: @rollup/plugin-typescript TS1128: Declaration or statement expected.
[warning] 410-410: @rollup/plugin-typescript TS1109: Expression expected.
[warning] 411-411: @rollup/plugin-typescript TS1109: Expression expected.
[warning] 412-412: @rollup/plugin-typescript TS1005: ':' expected.
[warning] 413-413: @rollup/plugin-typescript TS1005: ',' expected.
[warning] 415-415: @rollup/plugin-typescript TS1003: Identifier expected.
[warning] 415-415: @rollup/plugin-typescript TS1138: Parameter declaration expected.
[warning] 416-416: @rollup/plugin-typescript TS1003: Identifier expected.
[warning] 416-416: @rollup/plugin-typescript TS1138: Parameter declaration expected.
[warning] 419-419: @rollup/plugin-typescript TS1003: Identifier expected.
[warning] 419-419: @rollup/plugin-typescript TS1138: Parameter declaration expected.
[warning] 422-422: @rollup/plugin-typescript TS1005: ',' expected.
[warning] 434-434: @rollup/plugin-typescript TS1005: ',' expected.
[warning] 434-434: @rollup/plugin-typescript TS1005: ',' expected.
[warning] 434-434: @rollup/plugin-typescript TS1136: Property assignment expected.
[warning] 453-453: @rollup/plugin-typescript TS1128: Declaration or statement expected.
[warning] 459-459: @rollup/plugin-typescript TS1128: Declaration or statement expected.
…custom-fertilizer
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fdm-app/app/components/custom/fertilizer/columns.tsx (2)
32-35: Consider removing commented codeThere's a commented-out column definition for the "p_id" field. If this is intentionally excluded from the UI, consider removing the commented code entirely to keep the codebase clean.
- // { - // accessorKey: "p_id", - // header: "ID", - // },
64-98: Consider using a derived accessor for the Type columnThe "Type" column uses "Type" as the accessorKey, but there's no corresponding property in the
Fertilizertype. Consider using an accessor function to make this relationship more explicit:{ - accessorKey: "Type", + id: "type", + accessorFn: (row) => ({ + manure: row.p_type_manure, + compost: row.p_type_compost, + mineral: row.p_type_mineral + }), cell: ({ row }) => { const fertilizer = row.original return ( <span className="flex items-center gap-2"> {fertilizer.p_type_manure ? ( <Badge className="bg-amber-600 text-white hover:bg-amber-700" variant="default" > Mest </Badge> ) : null} {fertilizer.p_type_compost ? ( <Badge className="bg-green-600 text-white hover:bg-green-700" variant="default" > Compost </Badge> ) : null} {fertilizer.p_type_mineral ? ( <Badge className="bg-blue-600 text-white hover:bg-blue-700" variant="default" > Kunstmest </Badge> ) : null} </span> ) }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fdm-app/app/components/custom/fertilizer/columns.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
fdm-app/app/components/custom/fertilizer/columns.tsx (1)
fdm-app/app/components/custom/fertilizer/column-header.tsx (1)
DataTableColumnHeader(20-66)
🔇 Additional comments (3)
fdm-app/app/components/custom/fertilizer/columns.tsx (3)
1-11: Import section looks goodThe imports are properly structured and include all necessary components. The
NavLinkis correctly imported from "react-router-dom", addressing the issue mentioned in a previous review.
13-29: Type definition is complete and well-structuredThe
Fertilizertype definition now includes all required properties mentioned in previous reviews. Properties are appropriately marked as optional with proper type definitions, ensuring type safety throughout the component.
99-117: Details column implementation is goodThe "Details" column provides a good user experience with a tooltip and navigation link. The implementation is clean and follows best practices.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
fdm-core/src/catalogues.test.ts (1)
664-719:⚠️ Potential issueApply same fix to cultivation test.
The cultivation catalog test likely has the same issue as the fertilizer test. It selects a random item from the database rather than one that's guaranteed to be in the BRP catalog.
Modify the test using the same approach:
it("should update cultivation catalogue", async () => { await syncCatalogues(fdm) - // Update a catalogue item - const item = await fdm - .select({ - b_lu_catalogue: schema.cultivationsCatalogue.b_lu_catalogue, - hash: schema.cultivationsCatalogue.hash, - }) - .from(schema.cultivationsCatalogue) - .where(isNotNull(schema.cultivationsCatalogue.hash)) - .limit(1) - expect(item[0].b_lu_catalogue).toBeDefined() + // Start with a cultivation from the BRP catalogue + const brpCatalogue = await getCultivationCatalogue("brp") + const originalCatalogItem = brpCatalogue[0] // Use the first item + expect(originalCatalogItem).toBeDefined() + expect(originalCatalogItem.b_lu_catalogue).toBeDefined() + + // Find this item in the database + const item = await fdm + .select({ + b_lu_catalogue: schema.cultivationsCatalogue.b_lu_catalogue, + hash: schema.cultivationsCatalogue.hash, + }) + .from(schema.cultivationsCatalogue) + .where(eq(schema.cultivationsCatalogue.b_lu_catalogue, originalCatalogItem.b_lu_catalogue)) + .limit(1) + expect(item[0].b_lu_catalogue).toBeDefined()
🧹 Nitpick comments (6)
fdm-core/src/catalogues.ts (1)
377-422: Consider extracting common catalogue sync logic.Both
syncFertilizerCatalogueandsyncCultivationCatalogueshare very similar logic structures. Consider extracting the common pattern into a higher-order function to reduce duplication.Here's a suggested implementation:
async function syncGenericCatalogue<T, K extends keyof T>( fdm: FdmType, getCatalogue: () => Promise<T[]>, hashItem: (item: T) => Promise<string>, table: any, idField: K, errorMessage: string ) { const catalogue = await getCatalogue(); await fdm.transaction(async (tx) => { try { for (const item of catalogue) { const hash = await hashItem(item); const existing = await tx .select({ hash: table.hash }) .from(table) .where(eq(table[idField as string], item[idField])) .limit(1); if (existing.length === 0) { await tx.insert(table).values({ ...item, hash: hash, }); } else if ( existing[0].hash === null || existing[0].hash === undefined || existing[0].hash !== hash ) { await tx .update(table) .set({ hash: hash }) .where(eq(table[idField as string], item[idField])); } } } catch (error) { throw handleError(error, errorMessage); } }); } // Usage: async function syncFertilizerCatalogue(fdm: FdmType) { await syncGenericCatalogue( fdm, () => getFertilizersCatalogue("srm"), hashFertilizer, schema.fertilizersCatalogue, 'p_id_catalogue', "Exception for syncFertilizerCatalogue" ); }Also applies to: 424-470
fdm-data/src/cultivations/index.ts (1)
4-22: Update JSDoc to reflect async nature.The JSDoc comments should be updated to reflect that this function is now asynchronous and returns a Promise.
Update the JSDoc:
* @returns An array of `CatalogueCultivationItem` objects representing the * requested cultivation catalogue. +* @returns A Promise that resolves to an array of `CatalogueCultivationItem` objects. * @throws {Error} Throws an error if the provided `catalogueName` is not * recognized or supported. * * * @example * ```typescript -* const brpCatalogue = getCultivationCatalogue("brp"); +* const brpCatalogue = await getCultivationCatalogue("brp"); * console.log(brpCatalogue); * ```fdm-data/src/fertilizers/index.ts (1)
4-22: Update JSDoc to reflect async nature.The JSDoc comments should be updated to reflect that this function is now asynchronous and returns a Promise.
Update the JSDoc:
* @returns An array of `CatalogueFertilizerItem` objects representing the * requested fertilizer catalogue. +* @returns A Promise that resolves to an array of `CatalogueFertilizerItem` objects. * @throws {Error} Throws an error if the provided `catalogueName` is not * recognized or supported. * * * @example * ```typescript -* const srmCatalogue = getFertilizersCatalogue("srm"); +* const srmCatalogue = await getFertilizersCatalogue("srm"); * console.log(srmCatalogue); * ```fdm-data/src/fertilizers/hash.test.ts (1)
65-172: Comprehensive test coverage, but could be refactoredThe test coverage is thorough, successfully verifying that:
- Different fertilizer items produce different hashes
- Identical items yield the same hash
- Changes to boolean, numerical, and string values result in different hashes
However, there's significant repetition of large fertilizer objects across tests.
Consider using a factory function to reduce repetition:
+ function createTestFertilizer(overrides = {}): CatalogueFertilizerItem { + return { + p_id_catalogue: "test-id-1", + p_source: "test-source", + p_name_nl: "Test Fertilizer 1", + p_name_en: "Test Fertilizer (EN)", + p_description: "This is a test fertilizer", + p_dm: 37, + // ... rest of default properties + ...overrides + }; + } it("should generate different hashes when a string value changes", async () => { - const fertilizer1: CatalogueFertilizerItem = { - p_id_catalogue: "test-id-1", - // ... many repeated properties - } + const fertilizer1 = createTestFertilizer(); - const fertilizer2: CatalogueFertilizerItem = { - ...fertilizer1, - p_name_nl: "Updated Test Fertilizer Name", - } + const fertilizer2 = createTestFertilizer({ + p_name_nl: "Updated Test Fertilizer Name" + });Also applies to: 174-234, 236-297, 298-359, 360-421
fdm-data/src/fertilizers/index.test.ts (1)
33-33: Unusual async usage in describe block.The
describefunction doesn't typically take an async callback. While this may not cause immediate issues, it's unconventional and could lead to unexpected behavior as the Promise returned by the async function is not being awaited or handled in any way.-describe("getCatalogueSrm", async () => { +describe("getCatalogueSrm", () => {fdm-core/src/fertilizer.ts (1)
431-434: Consider adding more specific error messaging.The error message "Fertilizer does not exist in catalogue" could be more informative by including the fertilizer ID and farm ID to help with troubleshooting.
- throw new Error("Fertilizer does not exist in catalogue") + throw new Error(`Fertilizer with ID ${p_id_catalogue} does not exist in catalogue for farm ${b_id_farm}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
fdm-core/src/db/migrations/0002_kind_cardiac.sqlis excluded by!fdm-core/src/db/migrations/**fdm-core/src/db/migrations/meta/0002_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 (12)
.changeset/chubby-bushes-burn.md(1 hunks)fdm-core/src/catalogues.test.ts(1 hunks)fdm-core/src/catalogues.ts(2 hunks)fdm-core/src/db/schema.ts(2 hunks)fdm-core/src/fertilizer.ts(9 hunks)fdm-data/src/cultivations/catalogues/brp.ts(3 hunks)fdm-data/src/cultivations/index.test.ts(3 hunks)fdm-data/src/cultivations/index.ts(1 hunks)fdm-data/src/fertilizers/catalogues/srm.ts(3 hunks)fdm-data/src/fertilizers/hash.test.ts(1 hunks)fdm-data/src/fertilizers/index.test.ts(2 hunks)fdm-data/src/fertilizers/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/chubby-bushes-burn.md
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-core/src/db/schema.ts
🧰 Additional context used
🧬 Code Definitions (8)
fdm-data/src/cultivations/index.ts (2)
fdm-data/src/cultivations/d.ts (2)
CatalogueCultivationName(1-1)CatalogueCultivation(14-14)fdm-data/src/cultivations/catalogues/brp.ts (1)
getCatalogueBrp(15-48)
fdm-data/src/cultivations/catalogues/brp.ts (2)
fdm-data/src/cultivations/d.ts (1)
CatalogueCultivation(14-14)fdm-data/src/cultivations/hash.ts (1)
hashCultivation(4-25)
fdm-data/src/fertilizers/hash.test.ts (1)
fdm-data/src/fertilizers/hash.ts (1)
hashFertilizer(4-25)
fdm-data/src/fertilizers/index.ts (2)
fdm-data/src/fertilizers/d.ts (2)
CatalogueFertilizerName(1-1)CatalogueFertilizer(55-55)fdm-data/src/fertilizers/catalogues/srm.ts (1)
getCatalogueSrm(15-83)
fdm-core/src/catalogues.ts (4)
fdm-data/src/fertilizers/index.ts (1)
getFertilizersCatalogue(23-31)fdm-data/src/fertilizers/hash.ts (1)
hashFertilizer(4-25)fdm-data/src/cultivations/index.ts (1)
getCultivationCatalogue(23-32)fdm-data/src/cultivations/hash.ts (1)
hashCultivation(4-25)
fdm-data/src/cultivations/index.test.ts (2)
fdm-data/src/cultivations/catalogues/brp.ts (1)
getCatalogueBrp(15-48)fdm-data/src/cultivations/index.ts (1)
getCultivationCatalogue(23-32)
fdm-data/src/fertilizers/index.test.ts (2)
fdm-data/src/fertilizers/catalogues/srm.ts (1)
getCatalogueSrm(15-83)fdm-data/src/fertilizers/index.ts (1)
getFertilizersCatalogue(23-31)
fdm-core/src/fertilizer.ts (4)
fdm-core/src/authorization.d.ts (1)
PrincipalId(21-23)fdm-core/src/db/schema.ts (3)
farmsTypeInsert(32-32)fertilizersCatalogueTypeInsert(239-240)fertilizersCatalogueTypeSelect(237-238)fdm-core/src/authorization.ts (1)
checkPermission(155-239)fdm-core/src/error.ts (1)
handleError(15-31)
🪛 GitHub Actions: Tests
fdm-core/src/catalogues.test.ts
[error] 661-661: AssertionError: expected 'Updated hash' to be 'fac006e5' // Object.is equality
🔇 Additional comments (24)
fdm-core/src/catalogues.ts (2)
372-375: Refactoring to modular design.The refactoring of
syncCataloguesto call two specialized functions improves code organization and readability. This modular approach makes maintenance easier and follows good software design principles.
405-415:Details
✅ Verification successful
Verify hash update logic in catalog syncing.
The pipeline failure suggests issues with the hash update logic. When a database item differs from the catalog, ensure it's correctly matched and updated.
🏁 Script executed:
#!/bin/bash # Check the SRM catalog structure and the sync implementation echo "Examining the getFertilizersCatalogue implementation:" cat fdm-data/src/fertilizers/catalogues/srm.ts | grep -A 15 "export async function getCatalogueSrm" echo -e "\nChecking how hash comparisons are handled:" cat fdm-core/src/catalogues.ts | grep -A 10 "existing\\[0\\].hash === null" echo -e "\nLooking at relevant tests:" cat fdm-core/src/catalogues.test.ts | grep -A 20 "should update fertilizer catalogue"Length of output: 3227
Hash update logic verified as expected.
The condition for triggering the hash update—checking if the existing hash is null, undefined, or not matching the computed hash—appears to be correctly implemented for the fertilizers catalogue. The related tests infdm-core/src/catalogues.test.tsconfirm that the update behavior is triggered when the hash differs. Similarly, the analogous logic for the cultivations catalogue follows the same pattern.No further modifications to the hash update logic are required based on this verification. However, please ensure that additional test cases cover scenarios where the hash remains unchanged to prevent unnecessary updates.
fdm-core/src/catalogues.test.ts (3)
594-595: Correct usage of async function.Good update to add the
awaitkeyword when calling the now-asynchronousgetFertilizersCataloguefunction.
602-603: Correct usage of async function.Good update to add the
awaitkeyword when calling the now-asynchronousgetCultivationCataloguefunction.
606-662:⚠️ Potential issueFix failing test for fertilizer catalog update.
The pipeline shows this test is failing because after
syncCatalogues, the hash remains as "Updated hash" instead of reverting to the original value. This happens because the test selects a random item that may not be in the SRM catalog, so it isn't processed bysyncFertilizerCatalogue.Modify the test to use an item from the SRM catalog:
it("should update fertilizer catalogue", async () => { await syncCatalogues(fdm) - // Update a catalogue item - const item = await fdm - .select({ - p_id_catalogue: schema.fertilizersCatalogue.p_id_catalogue, - hash: schema.fertilizersCatalogue.hash, - }) - .from(schema.fertilizersCatalogue) - .where(isNotNull(schema.fertilizersCatalogue.hash)) - .limit(1) - expect(item[0].p_id_catalogue).toBeDefined() + // Start with a fertilizer from the SRM catalogue + const srmCatalogue = await getFertilizersCatalogue("srm") + const originalCatalogItem = srmCatalogue[0] // Use the first item + expect(originalCatalogItem).toBeDefined() + expect(originalCatalogItem.p_id_catalogue).toBeDefined() + + // Find this item in the database + const item = await fdm + .select({ + p_id_catalogue: schema.fertilizersCatalogue.p_id_catalogue, + hash: schema.fertilizersCatalogue.hash, + }) + .from(schema.fertilizersCatalogue) + .where(eq(schema.fertilizersCatalogue.p_id_catalogue, originalCatalogItem.p_id_catalogue)) + .limit(1) + expect(item[0].p_id_catalogue).toBeDefined()🧰 Tools
🪛 GitHub Check: core (20)
[failure] 661-661: src/catalogues.test.ts > Catalogues syncing > should update fertilizer catalogue
AssertionError: expected 'Updated hash' to be 'fac006e5' // Object.is equalityExpected: "fac006e5"
Received: "Updated hash"❯ src/catalogues.test.ts:661:36
🪛 GitHub Actions: Tests
[error] 661-661: AssertionError: expected 'Updated hash' to be 'fac006e5' // Object.is equality
fdm-data/src/cultivations/index.ts (1)
23-29: Proper conversion to async function.Good update to make
getCultivationCatalogueasynchronous, including the return type change toPromise<CatalogueCultivation>and the use ofawaitwhen callinggetCatalogueBrp().fdm-data/src/fertilizers/index.ts (1)
23-29: Proper conversion to async function.Good update to make
getFertilizersCatalogueasynchronous, including the return type change toPromise<CatalogueFertilizer>and the use ofawaitwhen callinggetCatalogueSrm().fdm-data/src/cultivations/catalogues/brp.ts (2)
2-2: Good addition of hashCultivation importThis import correctly supports the asynchronous hashing functionality that's being implemented.
15-16: Well-implemented async conversionThe function has been properly converted to an asynchronous implementation:
- Function signature now includes
asyncand returnsPromise<CatalogueCultivation>- Correctly uses async/await with the hashCultivation function
- Properly handles multiple concurrent operations with Promise.all
This change aligns with modern JavaScript/TypeScript practices for handling asynchronous operations, especially for potentially time-consuming operations like hashing.
Also applies to: 41-41, 46-46
fdm-data/src/fertilizers/hash.test.ts (1)
1-63: Well-structured test for hash generationThis test case thoroughly verifies that the hashFertilizer function generates a valid hash with the expected value.
fdm-data/src/cultivations/index.test.ts (3)
6-9: Properly updated async/await patternThe test has been correctly updated to handle async operations:
- Added
asyncto the test function- Used
awaitfor both function calls- Maintains the same assertion logic
12-16: Correct implementation of async error testingThe error handling test has been properly updated to use the
.rejects.toThrowError()pattern, which is the appropriate way to test for rejected promises in async/await code.
18-22: Well-structured async test conversionsAll test cases have been correctly updated to handle the asynchronous nature of the functions being tested, while maintaining the same test behavior and assertions.
Also applies to: 24-29, 31-38, 42-56, 58-61
fdm-data/src/fertilizers/catalogues/srm.ts (2)
2-2: Properly implemented async patternThe function has been correctly converted to an asynchronous implementation using the same pattern as in other files:
- Added appropriate import for hashFertilizer
- Updated function signature to return a Promise
- Properly uses await with hashFertilizer
- Correctly handles multiple promises with Promise.all
This ensures consistency across the codebase in how asynchronous operations are handled.
Also applies to: 15-16, 76-76, 81-82
68-68: Good property name consistency updateRenaming
p_cl_crtop_cl_rtimproves consistency with the field naming throughout the codebase, making maintenance easier.fdm-data/src/fertilizers/index.test.ts (2)
6-9: Good conversion to async/await pattern.The conversion of test cases to properly use async/await syntax is well-implemented. This ensures that promises are correctly resolved before assertions are made, preventing potential race conditions or false positives in tests.
Also applies to: 12-16, 19-20, 25-26, 35-36, 92-93
84-84: Property name update consistent with schema changes.The change from checking
p_cl_crtop_cl_rtaligns with the recent renaming in the schema definition files, ensuring consistency across the codebase.fdm-core/src/fertilizer.ts (7)
129-129:⚠️ Potential issueFix parameter name inconsistency.
There's a typo in the property name, which should be
p_cr_rtinstead ofpr_cr_rtto maintain naming consistency with other properties.- pr_cr_rt: schema.fertilizersCatalogueTypeInsert["p_cr_rt"] + p_cr_rt: schema.fertilizersCatalogueTypeInsert["p_cr_rt"]
133-133: Correctly updated parameter type reference.The property definition has been corrected to use the proper schema type
p_cl_rtinstead ofp_cl_cr, which aligns with the schema changes and ensures type consistency.
86-138: Improved function signature with proper security checks.The function now correctly requires
principal_idandb_id_farmparameters to enforce permission checks, and returns the created fertilizer ID. This enhances security and improves reusability by allowing callers to easily reference the newly created entity.
149-157: Well-implemented hash generation flow.The code properly builds the input object, sets the hash to null initially, then uses the imported
hashFertilizerfunction to generate a hash based on the complete object. This approach ensures data integrity and consistent hashing.
356-460: Well-designed update function with proper security.The new
updateFertilizerFromCataloguefunction is well-implemented with:
- Proper permission checks
- Verification that the fertilizer exists
- Secure filtering by both catalogue ID and farm ID (source)
- Proper hash regeneration after updates
This ensures data integrity and prevents unauthorized modifications.
1-13: Good import organization.The imports are well-organized with appropriate grouping and include the new dependencies needed for enhanced functionality (
andfrom drizzle-orm andhashFertilizerfrom the data package).
410-454: Consistent error handling pattern.The function follows the established pattern for error handling in the codebase, using the
handleErrorhelper function with specific error messages and context. This maintains consistency and provides useful debugging information.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fdm-data/src/cultivations/index.ts (1)
12-14: Remove the duplicate @returns tagThere are two @returns tags in the JSDoc documentation. The original tag on lines 12-13 should be removed to avoid confusion, leaving only the new Promise-based description.
* @param catalogueName - The name of the desired cultivation catalogue. * Currently supported names are: "brp". - * @returns An array of `CatalogueCultivationItem` objects representing the - * requested cultivation catalogue. * @returns A Promise that resolves to an array of `CatalogueCultivationItem` objects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
fdm-core/src/catalogues.test.ts(3 hunks)fdm-core/src/catalogues.ts(2 hunks)fdm-data/src/cultivations/index.ts(1 hunks)fdm-data/src/fertilizers/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-core/src/catalogues.test.ts
🧰 Additional context used
🧬 Code Definitions (3)
fdm-data/src/fertilizers/index.ts (2)
fdm-data/src/fertilizers/d.ts (2)
CatalogueFertilizerName(1-1)CatalogueFertilizer(55-55)fdm-data/src/fertilizers/catalogues/srm.ts (1)
getCatalogueSrm(15-83)
fdm-core/src/catalogues.ts (4)
fdm-data/src/fertilizers/index.ts (1)
getFertilizersCatalogue(24-32)fdm-data/src/fertilizers/hash.ts (1)
hashFertilizer(4-25)fdm-data/src/cultivations/index.ts (1)
getCultivationCatalogue(24-33)fdm-data/src/cultivations/hash.ts (1)
hashCultivation(4-25)
fdm-data/src/cultivations/index.ts (2)
fdm-data/src/cultivations/d.ts (2)
CatalogueCultivationName(1-1)CatalogueCultivation(14-14)fdm-data/src/cultivations/catalogues/brp.ts (1)
getCatalogueBrp(15-48)
🪛 GitHub Check: codecov/patch
fdm-core/src/catalogues.ts
[warning] 419-419: fdm-core/src/catalogues.ts#L419
Added line #L419 was not covered by tests
[warning] 467-467: fdm-core/src/catalogues.ts#L467
Added line #L467 was not covered by tests
🔇 Additional comments (13)
fdm-data/src/fertilizers/index.ts (2)
14-14: Documentation updates correctly reflect async changesThe updated JSDoc comments now correctly indicate that the function returns a Promise and the example shows proper usage with the
awaitkeyword.Also applies to: 20-20
24-26:Details
✅ Verification successful
Function correctly converted to async pattern
The function signature has been properly updated with the
asynckeyword and the correct return type ofPromise<CatalogueFertilizer>. The implementation correctly usesawaitwhen callinggetCatalogueSrm().Note that this is a breaking change for consumers of this API. All callers of
getFertilizersCataloguewill need to be updated to handle the Promise return type.Also applies to: 29-29
🏁 Script executed:
#!/bin/bash # Search for instances where getFertilizersCatalogue is called to ensure they're properly awaited rg -A 2 -B 2 "getFertilizersCatalogue" --glob "*.{ts,tsx,js,jsx}" --no-filenameLength of output: 2358
Async Conversion Verified – No Further Updates Required
The
getFertilizersCataloguefunction infdm-data/src/fertilizers/index.tshas been correctly updated to an asynchronous pattern with the proper return type. Verification across test cases confirms all invocations useawaitappropriately. Please note that this change is breaking for API consumers, so all external calls must be updated accordingly.
- Function signature correctly uses
asyncand returns aPromise<CatalogueFertilizer>.- Test files and example usages now await the function call as expected.
- Reminder: Update any external consumers if not already handled.
fdm-data/src/cultivations/index.ts (3)
20-20: LGTM: Example updated correctlyThe example has been properly updated to include the
awaitkeyword, reflecting the function's new asynchronous nature.
29-29: LGTM: Proper await usageThe
awaitkeyword has been correctly added to properly handle the Promise returned bygetCatalogueBrp().
24-26:Details
❓ Verification inconclusive
LGTM: Function signature updated properly
The function signature has been correctly updated to be asynchronous, matching the implementation in
getCatalogueBrp().
🏁 Script executed:
#!/bin/bash # Check for any usage of getCultivationCatalogue without await to identify potential breaking changes echo "Checking for usages of getCultivationCatalogue..." rg "getCultivationCatalogue\\((?!.*await)" --type tsLength of output: 427
Action Required: Re-verify Asynchronous Usage with Updated Regex Options
The asynchronous function signature in
fdm-data/src/cultivations/index.tshas been correctly updated to align withgetCatalogueBrp(). However, our initial automated check for direct invocations ofgetCultivationCataloguewithout usingawaitfailed due to unsupported lookahead in the regex engine. To proceed, please re-run the verification using the PCRE2 flag. For example:#!/bin/bash echo "Checking for usages of getCultivationCatalogue..." rg --pcre2 "getCultivationCatalogue\\((?!.*await)" --type tsOnce you confirm that all usages correctly handle the asynchronous call, this change can be approved.
fdm-core/src/catalogues.ts (8)
10-11: LGTM! Required imports added.The new imports for
hashCultivationandhashFertilizerare correctly added to support the functionality introduced in the new synchronization functions.
373-375: Excellent refactoring of the syncCatalogues function.The refactoring improves code organization by separating concerns into dedicated functions. This makes the code more maintainable and easier to understand.
377-422: The implementation of syncFertilizerCatalogue looks good.The function correctly:
- Fetches the SRM fertilizer catalogue
- Processes items within a transaction
- Updates or inserts catalogue items with proper hash calculations
- Properly implements error propagation as requested in a previous review
A test for the hash update logic was already provided in a previous review comment. Please ensure these tests are implemented to cover this code path, particularly line 419 which is flagged by code coverage tools.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 419-419: fdm-core/src/catalogues.ts#L419
Added line #L419 was not covered by tests
424-470: Well-implemented syncCultivationCatalogue function.The function follows the same pattern as syncFertilizerCatalogue with appropriate adaptations for cultivation data. The error handling is properly implemented as requested in a previous review.
Similar to syncFertilizerCatalogue, please ensure test coverage for the hash update logic and error handling path (line 467) which is flagged by code coverage tools.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 467-467: fdm-core/src/catalogues.ts#L467
Added line #L467 was not covered by tests
404-405: Hash update logic is well-implemented.The logic correctly identifies when a hash needs to be updated (null, undefined, or different) and updates only when necessary. This optimization avoids unnecessary database operations.
Also applies to: 407-411
450-453: Consistent hash update implementation across catalogue types.The hash update logic for cultivation catalogue follows the same pattern as for fertilizers, maintaining consistency across the codebase. This is a good practice for maintainability.
Also applies to: 455-462
467-467:Details
❓ Verification inconclusive
Error handling properly implemented for cultivation catalogue sync.
The error handling now correctly throws the handled error, consistent with the approach in syncFertilizerCatalogue.
Run the following script to verify test coverage for this error path:
🏁 Script executed:
#!/bin/bash # Check if there are tests that cover the error handling path rg -A 2 -B 2 "test.*syncCultivationCatalogue.*error" --type=ts fdm-core/srcLength of output: 77
SyncCultivationCatalogue Error Handling – Manual Test Coverage Verification Needed
The error handling implementation (i.e. throwing the handled error using
throw handleError(error, "Exception for syncCultivationCatalogue")) is consistent with the approach insyncFertilizerCatalogue. However, our automated search for tests covering this error path did not produce any results. Please manually verify that tests exist for this code path or consider adding appropriate tests if they are missing.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 467-467: fdm-core/src/catalogues.ts#L467
Added line #L467 was not covered by tests
419-419:Details
❓ Verification inconclusive
Error handling correctly propagates errors.
The function now properly throws the handled error, allowing it to be caught and handled by calling functions.
Run the following script to verify test coverage for this error path:
🏁 Script executed:
#!/bin/bash # Check if there are tests that cover the error handling path rg -A 2 -B 2 "test.*syncFertilizerCatalogue.*error" --type=ts fdm-core/srcLength of output: 76
Error handling in
syncFertilizerCatalogueappears correct as the handled error is properly thrown; however, our automated search did not uncover any tests covering this specific error path infdm-core/src/catalogues.ts(line 419). Please manually verify whether there are existing tests for this error scenario or add tests as needed.
- Confirm that tests exercise the error handling for
syncFertilizerCatalogue(look for any tests triggering an error and verifying it is correctly thrown).- If no tests exist, consider adding them to ensure robust error propagation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 419-419: fdm-core/src/catalogues.ts#L419
Added line #L419 was not covered by tests
| p_mg_rt: 180, | ||
| p_ca_rt: 190, | ||
| p_ne: 200, | ||
| p_s_rt: 210, |
There was a problem hiding this comment.
p_ne is a correct name?
There was a problem hiding this comment.
It stands for Neutralising effect of product with the unit g CaO/kg
2 participants
Summary by CodeRabbit
New Features
FarmHeadercomponent with fertilizer selection options.Improvements