-
Notifications
You must be signed in to change notification settings - Fork 102
Lf 4765 create get endpoint to fetch list of i ps #3755
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 43 commits
a959860
3b38070
0f9e03e
07c4a5b
645fd4d
549fd6c
1ed1420
8ca78c2
89fa6b0
d489db1
7149fa7
c4ffe7d
72ac94d
e6dbd57
a44f01d
7f62c17
498fb9f
74671e1
e60d300
36bbde8
ce0a969
f82a34a
f21d713
d9c6532
335ba39
c62de99
1cb661d
76e4fbc
2e90e3a
867842f
4860c06
406ca29
ed4a117
43899c1
f72a318
8e8c1f3
3940875
6aa5d17
6edc866
cda8040
e0dcf25
ff5aedf
7008086
e00783b
199247e
7927a3a
e7a9115
bf0310c
b461a6a
7dae074
86cbd5c
66b0f86
78d0cc3
7207d69
1e530ae
4871eb3
39172f8
8d54eab
3a6219f
6f14118
009e46a
42b9df0
8090cf4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||||||||
| const PARTNER_ID_MAP: Record<string, Partial<AddonFunctions>> = { | ||||||||
| '0': {}, | ||||||||
| '1': ESciAddon, | ||||||||
| }; | ||||||||
|
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); | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I am fairly certain we have been using 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 It feels like the end best solution will be that
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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!
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😄 :
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()]; | ||||||||
|
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(); | ||||||||
|
Duncan-Brain marked this conversation as resolved.
Outdated
|
||||||||
| } | ||||||||
| return res.status(200).send(irrigationPrescriptions); | ||||||||
| } else { | ||||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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({ | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think ideally Less of a big deal is that this is a GET, not a POST, to ESci, so I'm not sure
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ class IrrigationTypesModel extends BaseModel { | |
| type: 'object', | ||
| required: [''], | ||
| properties: { | ||
| irrigation_type_id: { type: 'string' }, | ||
| irrigation_type_id: { type: 'integer' }, | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' }, | ||
|
|
||
There was a problem hiding this comment.
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_MAPwithout the 0 key to start with? And clean up your type + type guards right away?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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?