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

229 adapt fertiscan to the new datastore with container #235

Draft
wants to merge 9 commits into
base: 224-enable-users-grouping-on-the-datastore
Choose a base branch
from

Conversation

Francois-Werbrouck
Copy link
Contributor

@Francois-Werbrouck Francois-Werbrouck commented Jan 29, 2025

This PR

Sub issues based on this PR (waiting to be merged):

What needs review:

@Francois-Werbrouck Francois-Werbrouck force-pushed the 229-adapt-fertiscan-to-the-new-datastore-with-container branch from 0a63bea to c919ea0 Compare February 7, 2025 21:28
@snakedye
Copy link

snakedye commented Feb 10, 2025

@Francois-Werbrouck InspectionController requires and the inspection_id and the folder_id even if neither are available because the Inspection isn't registered (InspectionController.register_analysis).

Is it possible to make both those arguments optional?

@Francois-Werbrouck Francois-Werbrouck linked an issue Feb 12, 2025 that may be closed by this pull request
@Francois-Werbrouck
Copy link
Contributor Author

@Francois-Werbrouck InspectionController requires and the inspection_id and the folder_id even if neither are available because the Inspection isn't registered (InspectionController.register_analysis).

This should be solved. The controller only takes the model which contain all the ids

@Francois-Werbrouck
Copy link
Contributor Author

Francois-Werbrouck commented Feb 12, 2025

All the basic work for the Rework the Fertiscan Inspection CRUD Workflow should be done. @snakedye Testing has been done on, therefor it should better work.

Side notes,

  • There has been some renaming:
def register_analysis(): #Previously the Create of CRUD
def new_inspection()->InspectionController: #New Create of CRUD
  • The Controller allow to manage an existing Inspection, but update require an updated model (it could simply be the one stocked in the controller but for flexibility we want to allow formatted dict too)
def __init__(self, inspection_model: Inspection):
def get_inspection_image_location_data(self, cursor: Cursor)->tuple[UUID,UUID]:
def update_inspection(self, cursor: Cursor, user_id: UUID, updated_data: dict | Inspection,)->Inspection:
async def delete_inspection(self, cursor:Cursor, user_id: UUID)-> DBInspection:
  • Deleting an Inspection also delete the pictures and folder related to the Inspection (lmk if yall think its problematic)

@Francois-Werbrouck Francois-Werbrouck force-pushed the 229-adapt-fertiscan-to-the-new-datastore-with-container branch from f079d0d to ca9ee34 Compare February 26, 2025 23:26
@Francois-Werbrouck Francois-Werbrouck marked this pull request as ready for review February 26, 2025 23:26
@Francois-Werbrouck Francois-Werbrouck requested a review from a team as a code owner February 26, 2025 23:26
@Francois-Werbrouck Francois-Werbrouck self-assigned this Feb 26, 2025
@Francois-Werbrouck Francois-Werbrouck marked this pull request as draft February 26, 2025 23:31
…nd-for-better-performance-and-scalability (#250)

* Issue #249: Moved DB new_inspection to python module

* Issue #249: Moved DB `update_inspection()` into python module

* Add error handeling to Ingredients

* Issue #249: Fertiscan Datastore tests passing

* Issue #249: All Query tests are passing

* Issue #249: Formatting
* Issue #233: Build doc

* Issue #233: `search_inspection` added

* Issue #233: Search modules tests passing

* Issue #233: datastore search_inspection test

* Issue #233: fix markdown lint

* Issue #233: Formatting

* Issue #233: Fix markdown lint

* Issue #233: fix high level doc
…ed in inspection for consistency (#259)

* Issue #252: updated_at added to the Inspection model

* Issue #252: fix lint
Copy link

@snakedye snakedye left a comment

Choose a reason for hiding this comment

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

LGTM so far. The new API is well suited for the backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In review
2 participants