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-4690 Create POST endpoint to create an Ensemble addon #3655

Draft
wants to merge 10 commits into
base: LF-4625-readability-refactors
Choose a base branch
from

Conversation

kathyavini
Copy link
Collaborator

@kathyavini kathyavini commented Jan 16, 2025

This controller was written to work on the current table. I'll draft it until the migration + models are merged and then update.

Description

This is the controller to link an Ensemble organization to a LiteFarm farm. You can send a different integrating_partner_id if you like but you'll get back a 400.

I'll point the base branch to my refactor branch as I used the cleaned up controller code a bit.

Jira link: https://lite-farm.atlassian.net/browse/LF-4690

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?

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

Link or update organization with a simple POST to /sensor/associate_esci_org/

Request should be (you can use any org UUID registered to our Ensemble user, and the one below does work)

{
  "integrating_partner_id": 1,
  "organization_uuid": "9f48cb5d-9f96-4e16-92f7-c680f27f09da"
}

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

… for readability

Remove reduce in favour of more readable array methods; encapsulate all three Ensemble methods into one function; define a constant for 'Ensemble Scientific'
…adability

Remove unused returned values from registerOrganizationWebhook; un-nest code run when function does not return early; add comments to make more explicit how bulkSensorClaim response is defined from Ensemble API response
@kathyavini kathyavini added the enhancement New feature or request label Jan 16, 2025
@kathyavini kathyavini self-assigned this Jan 16, 2025
@kathyavini kathyavini requested review from a team as code owners January 16, 2025 22:14
@kathyavini kathyavini requested review from antsgar and Duncan-Brain and removed request for a team January 16, 2025 22:14
const allRegisteredOrganizations = await getEnsembleOrganizations(access_token);

const organization = allRegisteredOrganizations.find(
({ uuid }) => uuid === organization_uuid,
Copy link
Collaborator Author

@kathyavini kathyavini Jan 16, 2025

Choose a reason for hiding this comment

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

This is where I would very much like to see a secondary check, maybe on email or farm name? But we would have to coordinate with Ensemble to make sure their organization registration included it.

import syncAsyncResponse from '../util/syncAsyncResponse.js';
import knex from '../util/knex.js';

const getSensorTranslations = async (language) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For some reason I am hesitant to just partially remove the CSV option haha. It mostly works but just doesn't meet ideal design and needs to be adapted for new fields maybe. I can't deny I am partial to leaving it in place for custom sensors 🥲

And maybe editing sensor controller right now won't be necessary if you agree that it is rest-ful to work on the /farm_external_integration resource instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll defer to @antsgar on the CSV upload; there will be no way to access it anymore from app is why I think it's okay to remove now, but you're right it's a partial removal as I only adjusted in this particular controller. That was just my understanding of the ticket requirements, but really there is a lot more code that can be deleted (from shared and webapp) if we are wiping CSV upload entirely. That deletion might be best in its own PR so even if we rebuild CSV addition from the ground up when we re-introduce it (which is what I believe we're going to do), we'll have just one place to look for all of the original code.

@kathyavini
Copy link
Collaborator Author

kathyavini commented Jan 20, 2025

With the move towards the farm-wide "add-on" concept (and confirmation we're sticking with sending only organization_uuid), I'm now more inclined to let this be a POST to something like /add-ons/ or /add-ons/ensemble although I think that has to be implemented alongside the table rename (if that happens).

Otherwise I think "linking" and "unlinking" should l be the wording used now (to match the views) so I have made that change in the meantime.

@kathyavini kathyavini marked this pull request as draft January 21, 2025 20:29
@kathyavini kathyavini changed the base branch from integration to LF-4625-readability-refactors January 23, 2025 20:35
@kathyavini kathyavini changed the base branch from LF-4625-readability-refactors to integration January 23, 2025 20:40
@kathyavini kathyavini changed the title LF-4625a Update controller for sensor addition LF-4690 Create POST endpoint to create an Ensemble addon Jan 23, 2025
@kathyavini kathyavini changed the base branch from integration to LF-4625-readability-refactors January 23, 2025 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants