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.

Sign up for GitHub

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

Issue #162: Add endpoint to retrieve pictures associated with an inspection #225

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Issue #162: Add endpoint to retrieve pictures associated with an inspection #225

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d8d334
Add endpoint to retrieve pictures associated with an inspection
snakedye Dec 18, 2024
f3a0e94
Refactor inspection picture retrieval to use new query function and u…
snakedye Dec 30, 2024
1c00dfb
Refactor get_pictures function to remove user dependency and update r…
snakedye Jan 2, 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
22 changes: 22 additions & 0 deletions app/controllers/inspections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
get_user_analysis_by_verified,
register_analysis,
)
from fertiscan.db.queries.inspection import get_inspection_dict as get_inspection
from datastore.db.queries.picture import (
get_picture_set_pictures
)
from fertiscan import update_inspection as db_update_inspection
from fertiscan.db.queries.inspection import (
InspectionNotFoundError as DBInspectionNotFoundError,
Expand Down Expand Up @@ -70,6 +74,24 @@ async def read_all_inspections(cp: ConnectionPool, user: User):

return inspections

async def get_pictures(cp: ConnectionPool, user: User, id: UUID | str):
"""
Retrieves the pictures associated with a user by inspection ID.
"""
if not user.id:
raise MissingUserAttributeError("User ID is required for fetching inspections.")
if not id:
raise ValueError("Inspection ID is required for fetching inspection details.")
if not isinstance(id, UUID):
id = UUID(id)

with cp.connection() as conn, conn.cursor() as cursor:
try:
inspection = get_inspection(cursor, id)
return get_picture_set_pictures(cursor, inspection['picture_set_id']) # TODO: This function will be deprecated
except DBInspectionNotFoundError as e:
log_error(e)
raise InspectionNotFoundError(f"{e}") from e

async def read_inspection(cp: ConnectionPool, user: User, id: UUID | str):
"""
Expand Down
15 changes: 15 additions & 0 deletions app/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
read_all_inspections,
read_inspection,
update_inspection,
get_pictures,
)
from app.controllers.users import sign_up
from app.dependencies import (
Expand Down Expand Up @@ -106,6 +107,20 @@ async def get_inspection(
status_code=HTTPStatus.NOT_FOUND, detail="Inspection not found"
)

@app.get("/inspections/{id}/pictures", tags=["Inspections"])
Copy link
Member

@k-allagbe k-allagbe Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I suggest get /files/<file_id>.
Because:

  • we also support pdfs I believe
  • we'll later handle files separately from inspections as their own resource
  • get inspections/<id> should return a list of file ids as part of the data. This is probably what we need to work on most.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, the pipeline does support pdfs. I don't think the refactored pipeline @saratavakoli77 and @Endlessflow are working on does. However, the application (frontend+backend) itself doesn't support sending pdfs and neither does the datastore.

The datastore can return a picture_set_id which I think is akin to a folder in a container (might change in the future), @Francois-Werbrouck can correct me on that. If we have that, I don't think we need to return a full list of files.

Copy link
Member

Choose a reason for hiding this comment

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

Still, a broader term than pictures might give us more cushion for later.

In the frontend, we need the images themselves (hence their ids), not so much their container or set. We want to be able to fetch the images from the backend individually, in parallel, etc.. not always as a set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to get @Francois-Werbrouck opinion on this because it's directly linked to the refactoring he's doing. Personally, I can't think of a use case where we'd benefit from fetching images individually. Even if you have the IDs, how do you know which to fetch?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. It's a matter of good design that allows for easy future improvement. In an optimization process in the frontend we might fetch individual images only when required instead of fetching them all at once.

Question for both of you: how complex would it be to implement that?

Copy link

Choose a reason for hiding this comment

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

how complex would it be to implement that[ get /files/<file_id>] ?

  1. The Datastore handles blob type of object which original types were previously only pictures/images so we defined the functions and object with the name Picture. Handeling pdf files shouldn't be a problem.
  2. We currently do not save the original type of object of the blob received, but it shouldn't be that complex to add.
  3. We currently allow to either fetch a file by id or fetch a folder content.
  4. I think we currently (and in a short future will have refactored ) allow fetching user's transaction content (all the files of an Inspection in Fertiscan's case)
---
title: Datastore Pydantic Models 
---
classDiagram

note for PictureMetadata "What is returned when fetching<br> a folder's content is a List of this"
class PictureMetadata{
UUID picture_id
str link
bytes blob
}

    class Folder {
        UUID id
        str name
        str path
        List~UUID~ children
        List~UUID~ pictures
        Optional~UUID~ parent_id
    }

    class Container {
        UUID id
        str name
        bool is_public
        str storage_prefix
        List~UUID~ group_ids
        List~UUID~ user_ids
        Dict~UUID, Folder~ folders
        str path
    }

class Client {
        UUID id
        str name
        bool is_public
        str storage_prefix
        Dict~UUID, Container~ containers
    }
Loading

get inspections/ should return a list of file ids as part of the data. This is probably what we need to work on most.

This soon will be answered but I dont see the necessity.
I dont get why we would want to make it more complex, when will we want for an Inspection to partially fetch the files of its list? If the BE wants to include parallelism, you can simply fetch the files individually.

we'll later handle files separately from inspections as their own resource

My opinion on the back end requesting just a bunch of files (pictures) by ID doesn't suit a need in the user-application, but I see some utility for our Data Analyst. However at this point I don't see much urgency in allowing fetching a bunch of files just by IDs with no regards for their placement.
I think sticking to either fetching one file or the entire folder content would be better for the Overall Datastore and having App specific fetching are what's most important at the moment on the application when fetching the files.
If the need for our DataAnalyst to fetch N files by IDs arise because they scraped the DB for specific files, I think it wont be that complex or time consuming to implement it at that point.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I'm putting this into formal request for this issue. In the frontend I need to obtain the file ids associated with an inspection. You guys decide how you want to implement it. I don't mind receiving a set / container id in the inspection data but the backend must allow me to get the individual file ids somehow, which will then be used to query the file data directly, however seen fit.

async def get_inspection_pictures(
cp: Annotated[ConnectionPool, Depends(get_connection_pool)],
user: Annotated[User, Depends(fetch_user)],
id: UUID,
):
try:
return await get_pictures(cp, user, id)
except InspectionNotFoundError:
raise HTTPException(
status_code=HTTPStatus.NOT_FOUND, detail="Inspection not found"
)



@app.post("/inspections", tags=["Inspections"], response_model=Inspection)
async def post_inspection(
Expand Down
55 changes: 55 additions & 0 deletions tests/test_inspections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
)

from app.controllers.inspections import (
get_pictures,
create_inspection,
delete_inspection,
read_all_inspections,
Expand Down Expand Up @@ -176,6 +177,60 @@ async def test_valid_inspection_id_calls_get_full_inspection_json(
)
self.assertIsInstance(inspection, Inspection)

@patch("app.controllers.inspections.get_pictures")
async def test_valid_inspection_id_calls_get_pictures(
self, mock_get_full_inspection_json
):
cp = MagicMock()
conn_mock = MagicMock()
cursor_mock = MagicMock()
conn_mock.cursor.return_value.__enter__.return_value = cursor_mock
cp.connection.return_value.__enter__.return_value = conn_mock

user = User(id=uuid.uuid4())
inspection_id = uuid.uuid4()

sample_inspection = {
"inspection_id": str(inspection_id),
"inspection_comment": "string",
"verified": False,
"company": {},
"manufacturer": {},
"product": {
"name": "string",
"label_id": str(uuid.uuid4()),
"registration_number": "2224256A",
"lot_number": "string",
"metrics": {
"weight": [],
"volume": {"edited": False},
"density": {"edited": False},
},
"npk": "string",
"warranty": "string",
"n": 0,
"p": 0,
"k": 0,
},
"cautions": {"en": [], "fr": []},
"instructions": {"en": [], "fr": []},
"guaranteed_analysis": {
"title": {"en": "string", "fr": "string"},
"is_minimal": False,
"en": [],
"fr": [],
},
}

mock_get_full_inspection_json.return_value = json.dumps(sample_inspection)

pictures = await get_pictures(cp, user, inspection_id)

mock_get_full_inspection_json.assert_called_once_with(
cursor_mock, inspection_id, user.id
)
self.assertIsInstance(pictures, [])

@patch("app.controllers.inspections.get_full_inspection_json")
async def test_inspection_not_found_raises_error(
self, mock_get_full_inspection_json
Expand Down
Loading