Skip to content
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

Lf 4694 create get endpoint to check if an ensemble addon exists b #3671

Conversation

Duncan-Brain
Copy link
Collaborator

@Duncan-Brain Duncan-Brain commented Jan 29, 2025

Description

Adds a get endpoint for addons. Returns 200 if an addon exists with addon_partner_id or if not specified. Return 404 if there is no addon present on farm.

Jira link: LF-4694

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain): Postman check

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

@kathyavini
Copy link
Collaborator

@Duncan-Brain yup I just made those same two files on my POST branch 😂 But it might still work out.

@Duncan-Brain Duncan-Brain marked this pull request as ready for review January 29, 2025 00:45
@Duncan-Brain Duncan-Brain requested review from a team as code owners January 29, 2025 00:45
@Duncan-Brain Duncan-Brain requested review from antsgar and kathyavini and removed request for a team January 29, 2025 00:45
@Duncan-Brain
Copy link
Collaborator Author

Updated to allow addon_partner_id not be specified - same response style.

@kathyavini I have never took notice of it before but we used two different function styles and I wanted to align them here.

I am thinking about updating to the style I copied -- I think I got lucky on which template file I used but I am solely basing my intent to align them on GPT response:

Screenshot 2025-01-31 at 9 24 56 AM

Any other thoughts before I go with the higher order function way?

@kathyavini
Copy link
Collaborator

@Duncan-Brain I have absolutely no preference when it comes to function styles 😂 addFarmAddon() used to live in the sensor controller before the new route and model is the only reason it's the way it is. Please feel free to refactor 👍 🙏

@@ -54,85 +54,87 @@ const Prices = React.lazy(() => import('../containers/Insights/Prices'));
const ExpiredTokenScreen = React.lazy(() => import('../containers/ExpiredTokenScreen'));
Copy link
Collaborator

@kathyavini kathyavini Jan 31, 2025

Choose a reason for hiding this comment

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

Oh no the pre-commit linting diff returns 🤣 We really have to work that out.

But also I can't figure out why is there a diff on webapp routes in this PR? What's the actual (non-linting) diff here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its on the merge commit... so maybe merge commits count as touching files? There is no diff from me... Linting or merge process must be undoing or redoing same thing as Sayakas ... I think I did just redo a pnpm i also so maybe there was an update to the linter rule? I have zero personal vscode plugins like prettier installed so it wouldn't be that.

Copy link
Collaborator

@kathyavini kathyavini Jan 31, 2025

Choose a reason for hiding this comment

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

Okay, I guess it's getting committed, then! 😂

I'll just ping @antsgar because it looks like it was her merge commit that commited the current state. Anto you weren't at our tech daily where we looked at this based on Sayaka's PR with the same diff, but the ESLint rule running on commit is making the diff on the right, while prettier linting (format on save) according to our .prettierrc does the diff on the left. So if you work on this file you get the left diff, and when you run the pre-commit hook it will transform to the right -- kind of obnoxious 😝 Is it possible your pre-commit lint was off at one point doing the unused animals bit?

Anyway I think it would better if prettier + eslint weren't at odds in this way -- I think there should be some way to make prettier eslint aware (or vice versa) that we aren't implementing properly but in any case let's commit and sorry @SayakaOno I made you revert this earlier 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, thank you for finding that commit! I'm making some changes to the file again, and I'll leave it this time! 😂

Copy link
Collaborator

@antsgar antsgar Feb 4, 2025

Choose a reason for hiding this comment

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

Oh no, how annoying 😓 I always get the diff on the left no matter what, even if I change something on this file and format on save and then commit it

Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

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

It looks good!

I think we also need to get the org_uuid back though so the component can display it.

const { addon_partner_id } = req.query;
const rows = await FarmAddonModel.query()
.where({ farm_id, addon_partner_id })
.skipUndefined();
Copy link
Collaborator

@kathyavini kathyavini Feb 3, 2025

Choose a reason for hiding this comment

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

.skipUndefined() is a nice inclusion and should probably be our default!

@Duncan-Brain
Copy link
Collaborator Author

Duncan-Brain commented Feb 4, 2025

Okay, I wasn't aware Loic decided on a design to definitely include org uuid yet. And thought we were doing the status thing.

How would you like the response @kathyavini and @SayakaOno

id agnostic (like default animal type and breed) - if we go this route we can consider sending name of addon_partner instead of id:

// req includes addon_partner_id
["dsfdsf-adfsadf-asdfsf-adfsd", "dsfasfdsa-asdfsf-asdfdsa"]

// req does not include addon_partner_id
{
"Ensemble Scientific": ["dsfdsf-adfsadf-asdfsf-adfsd", "dsfasfdsa-asdfsf-asdfdsa"],
"No Integrating Partner": ["dsfdsf-adfsadf-asdfsf-adfsd", "dsfasfdsa-asdfsf-asdfdsa"]
}

or using using table ids

// req includes addon_partner_id
["dsfdsf-adfsadf-asdfsf-adfsd", "dsfasfdsa-asdfsf-asdfdsa"]

// req does not include addon_partner_id
{
"1": ["dsfdsf-adfsadf-asdfsf-adfsd", "dsfasfdsa-asdfsf-asdfdsa"],
"0": ["dsfdsf-adfsadf-asdfsf-adfsd", "dsfasfdsa-asdfsf-asdfdsa"]
}

or plain rows

// req includes addon_partner_id
[{org_uuid: "dsfdsf-adfsadf-asdfsf-adfsd"}, {org_uuid: "dsfdsf-adfsadf-asdfsf-adfsd"}]

// req does not include addon_partner_id
[{addon_partner_id: 1,  org_uuid: "dsfdsf-adfsadf-asdfsf-adfsd"}, {addon_partner_id: 0, org_uuid: "dsfdsf-adfsadf-asdfsf-adfsd"}]

My preference would be the first to continue our efforts to keep frontend id agnostic and send the name (analogous to "translation key") instead.

@kathyavini
Copy link
Collaborator

It was discussed at Tech Daily today (Feb 4th) but for our records, it was decided we like option (3) above -- array of objects which correspond to the actual record shape 🙏

Copy link
Collaborator Author

@Duncan-Brain Duncan-Brain left a comment

Choose a reason for hiding this comment

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

Okay it should be updated! We were so into formatting into entities on backend that I thought for sure we were sticking with it. My new favorite is option 3.

Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

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

Looks great. I'll merge as soon as the integration TypeScript fix is merged backend Docker container fix is merged (sorry for the moving target 🙇)

@kathyavini kathyavini added this pull request to the merge queue Feb 6, 2025
Merged via the queue into integration with commit 0c3963c Feb 6, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants