Skip to content
Merged
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
a959860
LF-4765 Add get prescriptions route to routes
Duncan-Brain Apr 23, 2025
3b38070
LF-4765 Add get prescriptions wireframe to controller
Duncan-Brain Apr 23, 2025
0f9e03e
LF-4765 WIP - add basic mock and get farmaddon function
Duncan-Brain Apr 24, 2025
07c4a5b
LF-4765 send response for postman testing
Duncan-Brain Apr 25, 2025
645fd4d
LF-4765 Add blocked model util
Duncan-Brain Apr 25, 2025
549fd6c
LF-4765 update mocking function
Duncan-Brain Apr 25, 2025
1ed1420
LF-4765 Was confused about which route I should use moved irrigation …
Duncan-Brain Apr 25, 2025
8ca78c2
LF-4765 Add dayjs package
Duncan-Brain Apr 28, 2025
89fa6b0
LF-4765 Add model function to return utc_offset
Duncan-Brain Apr 28, 2025
d489db1
LF-4765 Add new farm service and add farm specific date utils
Duncan-Brain Apr 29, 2025
7149fa7
LF-4765 Add type guard and change syntax for task_id
Duncan-Brain Apr 29, 2025
c4ffe7d
LF-4765 Add ensemble call to service and remove and export mocked fun…
Duncan-Brain Apr 29, 2025
72ac94d
LF-4765 Copy irrigation prescription request fromat for returning moc…
Duncan-Brain Apr 29, 2025
e6dbd57
LF-4765 Update task model with jsdoc, uncomment external id info now …
Duncan-Brain May 1, 2025
a44f01d
LF-4765 Export point type and update type guard to only test external…
Duncan-Brain May 1, 2025
7f62c17
LF-4765 Finally stop cheating, just adding full types
Duncan-Brain May 1, 2025
498fb9f
LF-4765 Add task id search to data formatting, refactor some types an…
Duncan-Brain May 1, 2025
74671e1
LF-4765 Filter tasks by farm Id since org_id are not constrained to j…
Duncan-Brain May 2, 2025
e60d300
LF-4765 update scope check
Duncan-Brain May 2, 2025
36bbde8
LF-4765 Fix data type in irrigation types
Duncan-Brain May 2, 2025
ce0a969
LF-4765 Add more types to type file
Duncan-Brain May 2, 2025
f82a34a
LF-4765 Update model function to return all tasks, update ensemble se…
Duncan-Brain May 2, 2025
f21d713
LF-4765 Add tests for get endpoint
Duncan-Brain May 2, 2025
d9c6532
LF-4765 Add after date to request query, remove farmDate utilities, a…
Duncan-Brain May 2, 2025
335ba39
LF-4765 Add after date to tests
Duncan-Brain May 2, 2025
c62de99
LF-4765 Cleanup duplicated types
Duncan-Brain May 2, 2025
1cb661d
LF-4765 Remove mistakenly added function
Duncan-Brain May 6, 2025
76e4fbc
LF-4765 Farm date cleanup
Duncan-Brain May 6, 2025
2e90e3a
LF-4765 Remove extra mocking function and move to tests, add todo for…
Duncan-Brain May 6, 2025
867842f
LF-4765 improve/fix mock logic
Duncan-Brain May 7, 2025
4860c06
LF-4765 Adjust frontend query to backend specs, move mocks to backend
Duncan-Brain May 7, 2025
406ca29
LF-4765 Update date params for irrigation prescription endpoints
Duncan-Brain May 7, 2025
ed4a117
LF-4765 Add management plan type and dependencies
Duncan-Brain May 8, 2025
43899c1
LF-4765 Add mamangement plan to expected return mock and type
Duncan-Brain May 8, 2025
f72a318
LF-4765 rename what was a temporary variable to a permanent one
Duncan-Brain May 8, 2025
8e8c1f3
LF-4765 Update types file with farm addon and point
Duncan-Brain May 8, 2025
3940875
LF-4765 Invalidate irrigation prescription query between logouts
Duncan-Brain May 8, 2025
6aa5d17
LF-4765 Send empty array if no farm addons that provide irrigation pr…
Duncan-Brain May 8, 2025
6edc866
LF-4765 Add frontend date utils to backend tests utils
Duncan-Brain May 9, 2025
cda8040
LF-4765 Fix tests to accomodate new values
Duncan-Brain May 9, 2025
e0dcf25
Merge branch 'integration' into LF-4765-create-get-endpoint-to-fetch-…
Duncan-Brain May 9, 2025
ff5aedf
LF-4765 Fix package lock
Duncan-Brain May 9, 2025
7008086
LF-4765 Touch up comments and types
Duncan-Brain May 9, 2025
e00783b
LF-4765 Add query check middleware
Duncan-Brain May 15, 2025
199247e
LF-4765 Update irrigation prescription types and type guard to use ne…
Duncan-Brain May 15, 2025
7927a3a
LF-4765 Small fixes to update comments, error array and accessing rec…
Duncan-Brain May 15, 2025
e7a9115
LF-4765 Add comments to regex check for use when testing with postman
Duncan-Brain May 15, 2025
bf0310c
LF-4765 Update jsdoc
Duncan-Brain May 15, 2025
b461a6a
LF-4765 Refactor tests proper by creating facotries and mocking the e…
Duncan-Brain May 16, 2025
7dae074
LF-4765 Make required query params in controller move type owner
Duncan-Brain May 16, 2025
86cbd5c
LF-4765 Add query params to frontend
Duncan-Brain May 16, 2025
66b0f86
LF-4765 Refactor mocking
Duncan-Brain May 16, 2025
78d0cc3
LF-4765 undo type casting
Duncan-Brain May 20, 2025
7207d69
LF-4765 Invalidate tags on task deletion and creation
Duncan-Brain May 20, 2025
1e530ae
LF-4765 Plural url form
Duncan-Brain May 20, 2025
4871eb3
LF-4765 Add addonPartner service
Duncan-Brain May 20, 2025
39172f8
LF-4765 Move types and data formatting to service
Duncan-Brain May 20, 2025
8d54eab
LF-4765 Move shouldSend boolean to only affect ESci
Duncan-Brain May 20, 2025
3a6219f
LF-4765 rename mock
Duncan-Brain May 20, 2025
6f14118
Merge branch 'integration' into LF-4765-create-get-endpoint-to-fetch-…
Duncan-Brain May 20, 2025
009e46a
Merge branch 'integration' into LF-4765-create-get-endpoint-to-fetch-…
kathyavini May 21, 2025
42b9df0
LF-4765 Wrong type for shouldSend
Duncan-Brain May 21, 2025
8090cf4
Merge branch 'integration' into LF-4765-create-get-endpoint-to-fetch-…
Duncan-Brain May 23, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 100 additions & 0 deletions packages/api/src/controllers/irrigationPrescriptionController.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/*
* Copyright 2025 LiteFarm.org
* This file is part of LiteFarm.
*
* LiteFarm is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* LiteFarm is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details, see <https://www.gnu.org/licenses/>.
*/

import { Response } from 'express';
import { LiteFarmRequest, HttpError } from '../types.js';
import ESciAddon from '../util/ensembleService.js';
import { fakeIrrigationPrescriptions } from '../../tests/utils/ensembleUtils.js';
import FarmAddonModel from '../models/farmAddonModel.js';
import { AddonFunctions, IrrigationPrescription } from '../util/ensembleService.types.js';

interface IrrigationPrescriptionQueryParams {
startTime?: string;
endTime?: string;
shouldSend?: string;
}

// TODO: LF-4710 - Delete partner_id = 0, remove Partial

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the cleanup old sensors implementation that Sayaka is working on now? Since that ticket is now in parallel development, would it be okay to just write the PARTNER_ID_MAP without the 0 key to start with? And clean up your type + type guards right away?

@Duncan-Brain Duncan-Brain May 15, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, I kind of still wonder if we will keep some of it -- the .csv upload seems useful for the .csv upload part of soil sample locations for soil amendment 2? Nvm saw no more researcher csv stuff today in grooming

@SayakaOno are you going to delete the database records for AddonPartner = 0, and FarmAddon.addon_partner_id = 0 in your PR? Do you think I should delete this here?

const PARTNER_ID_MAP: Record<string, Partial<AddonFunctions>> = {
'0': {},
'1': ESciAddon,
};
Comment thread
Duncan-Brain marked this conversation as resolved.
Outdated

const irrigationPrescriptionController = {
getPrescriptions() {
return async (req: LiteFarmRequest<IrrigationPrescriptionQueryParams>, res: Response) => {
try {
const { farm_id } = req.headers;
const { startTime, endTime, shouldSend } = req.query;
const irrigationPrescriptions: IrrigationPrescription[] = [];
const partnerErrors: unknown[] = [];

if (shouldSend === 'true') {
// Check for registered farm addons (only esci for now)
// @ts-expect-error - farm_id is guaranteed here by the checkScope middleware with single argument
const farmAddonPartnerIds = await FarmAddonModel.getDistinctFarmAddonPartnerIds(farm_id);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you know for sure farm_id won't be undefined, instead of @ts-expect-error you can use a non-null assertion operator

Suggested change
// @ts-expect-error - farm_id is guaranteed here by the checkScope middleware with single argument
const farmAddonPartnerIds = await FarmAddonModel.getDistinctFarmAddonPartnerIds(farm_id);
const farmAddonPartnerIds = await FarmAddonModel.getDistinctFarmAddonPartnerIds(farm_id!);

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0.html#non-null-assertion-operator

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like having a comment for unclear assertions. Assertions should be used sparingly.

I have a test branch for a specific fix for this open mentioned with Joyces comment: #3755 (comment)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say ts-expect-error should be used even more sparingly, and my understanding is that it's meant to be used temporarily. If you're absolutely sure farm_id would be present I think the non-null assertion would be okay. I did look at the other branch, and although I like the idea of adding a type guard I honestly feel it overcomplicates the code with not that much added benefit. But I'm curious to know what others think too

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a point we disagree on.

For code reuse and readability for devs, if I were to copy this code I would likely not notice a trailing ! present in the code vs a commented @ts-expect-error. It is a single character and assertions are dangerous because they override some typescript typings.

I am fairly certain we have been using @ts-expect-error as a stop gap when we have not found the correct typescript solution to make it work. The non-null assertion feels like a final 'correct' solution in limited circumstances. So, if I copied this controller code erroneously without a checkScope in the accompanying routes file -- the assertion could be incorrect.

In the other branch -- I don't know what type guard you are talking about -- but the explicit type assertion that overrides the express Request type is placed next to the checkScope to make it clear they go together. This is an assertion done in the routes context right next to the checkScope it relies upon... still not ideal but avoids the @ts-expect-error. In the link you provided from Typescript they also put the non-null assertion right next to some context claiming it is correctly validated:

function processEntity(e?: Entity) {
  validateEntity(e);
  let s = e!.name; // Assert that e is non-null and access name
}

It feels like the end best solution will be that express needs to make some sort of generic that goes along with type NextFunction or something like that but thats not our problem. Maybe the cleanest/safest way for us right now is to double up on certain type guards and put them in the controller as well as well as the middleware? Do you like that any better?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the type guard I'm referring to https://github.com/LiteFarmOrg/LiteFarm/pull/3772/files#diff-31ffdd44c889de461e56828613f90ff13ba191db724cda9bdb7b6e90e8937d48R45-R52.

A type assertion is as bad as a non-null assertion (arguably worse?) so my feeling is the balance between having a +185 -42 diff to end up with a solution that isn't ideal vs achieving a solution that isn't good either but just uses one character doesn't add up. I'd be totally okay with keeping the @ts-expect-error as a non-ideal solution too, that's not a blocker for me!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I would like to keep the ts-expect error then since it has a comment explaining its existence and we may not know the best way to type it.

Thanks for sharing the type guard, I think the one you are pointing too has nothing to do with the farm_id, the query param type guard check has been moved to middleware in this PR but in the draft PR it is outdated.

To be fair the actual code change to assert the correct type is like ~ +20, the rest is changing a core file to typescript and adding more types which is generally useful. I am really not saying we should do it but it seems like we are striving to have mostly correct typings -- I personally think this solution is possibly the most correct we can get with express.

To continue the best practice discussion -- because I like chatting about it 😄 :

A type assertion is as bad as a non-null assertion (arguably worse?)

Its not that either asserting that is necessarily bad, it is asserting out of context or to use as a temporary solution that is bad. There are genuine cases for each but being extra careful about both is important I think. For this case I think we would be using it here as a temporary solution, out of context, that could lead to serious code reuse errors.

The farm_id type guard that already exists in checkScope is two files away from the controller. If it were right beside it and not working in context (like the TS example, and other examples on internet I can find) I would agree non-null assertion.

Type casting assertions are also useful when maybe you cannot type check like external package or endpoint.


// Return empty array if no addons
if (!farmAddonPartnerIds.length) {
return res.status(200).send(irrigationPrescriptions);
}

// Loop through addon partners
for (const farmAddonPartnerId of farmAddonPartnerIds) {
try {
const addonPartner = PARTNER_ID_MAP[farmAddonPartnerId.addon_partner_id.toString()];
Comment thread
Duncan-Brain marked this conversation as resolved.
Outdated
// TODO: LF-4710 - Skip deprecated partner_id = 0 situation
// Type guard for undefined functions
if (!addonPartner || typeof addonPartner.getIrrigationPrescriptions !== 'function') {
continue;
}

irrigationPrescriptions.push(
// @ts-expect-error - farm_id is guaranteed here by the checkScope middleware with single argument
...(await addonPartner.getIrrigationPrescriptions(farm_id, startTime, endTime)),
);
} catch (error) {
partnerErrors.push(error);
}
}

// Return an error if there are no prescriptions
if (!irrigationPrescriptions.length && partnerErrors.length) {
throw partnerErrors.shift();
Comment thread
Duncan-Brain marked this conversation as resolved.
Outdated
}
return res.status(200).send(irrigationPrescriptions);
} else {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be deleted...

// Return data for dev purposes + QA
const mockData = await fakeIrrigationPrescriptions({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ideally shouldSend would control only whether ESci is contacted, but with this setup, not passing shouldSend = true means your real controller code (the code block above, and the ESci service function) will never be hit at all. I think I'll leave a comment at the end, but it would be better for our testing phase if the mock could replace just the ESci call, not the entire controller logic.

Less of a big deal is that this is a GET, not a POST, to ESci, so I'm not sure shouldSend is the right boolean. But I get the idea!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only affects the Esci endpoint now. Let me know if it works for you.

// @ts-expect-error - farm_id is guaranteed here by the checkScope middleware with single argument
farmId: farm_id,
startTime,
endTime,
});
return res.status(200).send(mockData);
}
} catch (error: unknown) {
console.error(error);
const err = error as HttpError;
const status = err.status || err.code || 500;
return res.status(status).json({
error: err.message || err,
});
}
};
},
};

export default irrigationPrescriptionController;
Original file line number Diff line number Diff line change
Expand Up @@ -13,28 +13,26 @@
* GNU General Public License for more details, see <https://www.gnu.org/licenses/>.
*/

import { Request, Response } from 'express';
import { Response } from 'express';
import { getOrgLocationAndCropData, sendFieldAndCropDataToEsci } from '../util/ensembleService.js';
import { LiteFarmRequest } from '../types.js';

interface HttpError extends Error {
status?: number;
code?: number; // LF custom error
}

interface LiteFarmQuery {
interface InitiateFarmIrrigationPrescriptionQueryParams {
allOrgs?: string;
shouldSend?: string;
}

export interface LiteFarmRequest extends Request<unknown, unknown, unknown, LiteFarmQuery> {
Comment thread
Duncan-Brain marked this conversation as resolved.
headers: Request['headers'] & {
farm_id?: string;
};
}

const irrigationPrescriptionRequestController = {
initiateFarmIrrigationPrescription() {
return async (req: LiteFarmRequest, res: Response) => {
return async (
req: LiteFarmRequest<InitiateFarmIrrigationPrescriptionQueryParams>,
res: Response,
) => {
const { farm_id } = req.headers;
const { allOrgs, shouldSend } = req.query;

Expand Down
13 changes: 13 additions & 0 deletions packages/api/src/models/farmAddonModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,19 @@ class FarmAddon extends baseModel {
.where('addon_partner_id', addonPartnerId)
.whereNotDeleted();
}

/**
* Retrieves all organization identifiers (uuid, pk) for a given addon partner.
*
* @param {string} farmId - The ID of the farm.
* @returns {Promise<Array<{addon_partner_id: number}>>} The organization identifiers and the farm they are associated with
*/
static async getDistinctFarmAddonPartnerIds(farmId) {
return FarmAddon.query()
.distinct('addon_partner_id')
.where('farm_id', farmId)
.whereNotDeleted();
}
}

export default FarmAddon;
2 changes: 1 addition & 1 deletion packages/api/src/models/irrigationTypesModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class IrrigationTypesModel extends BaseModel {
type: 'object',
required: [''],
properties: {
irrigation_type_id: { type: 'string' },
irrigation_type_id: { type: 'integer' },

@Duncan-Brain Duncan-Brain May 9, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error in model setup -- type is actually incrementing pk

irrigation_type_name: { type: 'string' },
farm_id: { type: 'string' },
default_measuring_type: { type: 'string' },
Expand Down
30 changes: 26 additions & 4 deletions packages/api/src/models/taskModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -360,16 +360,19 @@ class TaskModel extends BaseModel {
*/
static async getUnassignedTasksDueThisWeekFromIds(taskIds, isDayLaterThanUTC = false) {
const dayLaterInterval = isDayLaterThanUTC ? '"1 day"' : '"0 days"';
return await TaskModel.query().select('*').whereIn('task_id', taskIds).whereRaw(
`
return await TaskModel.query()
.select('*')
.whereIn('task_id', taskIds)
.whereRaw(
`
task.assignee_user_id IS NULL
AND task.complete_date IS NULL
AND task.abandon_date IS NULL
AND task.due_date <= (now() + ('1 week')::interval + (?)::interval)::date
AND task.due_date >= (now() + (?)::interval)::date
`,
[dayLaterInterval, dayLaterInterval],
);
[dayLaterInterval, dayLaterInterval],
);
}

/**
Expand Down Expand Up @@ -510,6 +513,25 @@ class TaskModel extends BaseModel {
.withGraphFetched('[animals(selectId), animal_batches(selectId)]')
.whereIn('task_id', taskIds);
}

/**
* Returns farm tasks where not deleted that has an external id
*
* @param {string} farmId - the farm requesting irrigation tasks
* @param {number[]} externalIds - the farm requesting irrigation tasks
* @static
* @async
* @returns {import('./types.js').IrrigationTask[]} - Object array with task id property only.
*/
static async getIrrigationTasksWithExternalIdByFarm(farmId, externalIds) {
return await TaskModel.query()
.select('task.*')
.withGraphJoined('[locations, irrigation_task]')
.whereNotNull('irrigation_task.irrigation_prescription_external_id')
.whereIn('irrigation_task.irrigation_prescription_external_id', externalIds)
.where('locations.farm_id', farmId)
.whereNotDeleted();
}
}

export default TaskModel;
Loading